Limit Order Bug Bounty

levx.io · October 22, 2020, 6:42am

What to investigate

All contracts in our /contracts in the repo https://github.com/sushiswap/sushiswap-settlement are eligible for the bounty.

Bounty Size

The size of the bounty will vary depending on the severity of the issue discovered. The severity is calculated according to the OWASP risk rating model based on Impact and Likelihood.

Critical: up to 10000 SUSHI
High: up to 5000 SUSHI
Medium: up to 500 SUSHI
Low: up to 100 SUSHI

Submission

If you found a vulnerability, leave a reply with this form:

Summary
Reproduce Steps
URL of source code and line number (optional)
Token Names (optional)

If we cannot reproduce an issue we will not be able to reward it.

Other considerations

In addition to severity, other variables are also considered:

Quality of description. Higher rewards are paid for clear, well-written submissions.
Quality of reproducibility. Please include test code, scripts and detailed instructions. The easier it is for us to reproduce and verify the vulnerability, the higher the reward.
Quality of fix, if included. Higher rewards are paid for submissions with clear description of how to fix the issue.

BoringCrypto · October 22, 2020, 7:40am

Critical issue:
Cancelled orders are deleted from the testnet. However, a relayer could simply cache the orders and still execute them, even after they are cancelled. This leads to unexpected orders executing and maybe a users makes and cancels 5 orders and later finds all 5 executed, leading to unwanted trades.

Fix: provide on-chain mapping of cancelled hashes. Already discussed with LevX.

Minor issue:

github.com

sushiswap/sushiswap-settlement/blob/master/contracts/Settlement.sol#L104


                args.order.fromToken,
                args.order.maker,
                msg.sender,
                fee
            );

            emit OrderFeeTransferred(hash, msg.sender, fee);
        }

        // Update order status
        filledAmountInOfHash[hash] = filledAmountInOfHash[hash] + args.amountToFillIn;

        emit OrderFilled(hash, args.amountToFillIn, amountOut);
    }
}

function _validateArgs(FillOrderArgs memory args, bytes32 hash) internal pure returns (bool) {
    return
        args.order.maker != address(0) &&
        args.order.fromToken != address(0) &&
        args.order.toToken != address(0) &&

Potential future reentrancy issue: The amount of the order that was filled doesn’t get updated until after external calls. When a user would place a limit buy order for a malicious token contract, this contract could keep executing the order multiple times until the user runs out of funds. So if the user has $10M USDT and puts in a limit order of $5k for a malicious token and the UI did an unlimited approve (pretty standard), the token contract could mint tokens, flashloan USDT, swap up to $10M worth, repay the loan and walk away with $10M in funds.

However, this doesn’t work, because the swap function of SushiSwap has reentrancy protection. But if pointed at a different factory contract or after future updates, this could become an issue, so for safety this should probably be corrected.

Fix: Move the statement up, before the external calls. Already discussed with LevX.

levx.io · October 26, 2020, 2:18am

First issue has been fixed: https://github.com/sushiswap/sushiswap-settlement/commit/b95c1f873f7ed9d76757757d486e8b4b88e2e19f

Second one isn’t an issue.