search

code-423n4 / 2021-09-sushitrident-findings

0 stars 0 forks source link

Add isWhiteListed(..)? #14

Closed code423n4 closed 3 years ago

code423n4 commented 3 years ago

Handle

gpersoon

Vulnerability details

Impact

Some of the functions of TridentRouter.sol contain the check: isWhiteListed(..). This includes:

However the following similar functions don't contain isWhiteListed(..):

This doesn't seem logical.

Proof of Concept

https://github.com/sushiswap/trident/blob/master/contracts/TridentRouter.sol function exactInputSingle(ExactInputSingleParams calldata params) public payable returns (uint256 amountOut) { .. // No isWhiteListed

function exactInput(ExactInputParams calldata params) public payable returns (uint256 amountOut) {
        ...
        isWhiteListed(params.path[i].pool);

function exactInputLazy(uint256 amountOutMinimum, Path[] calldata path) public payable returns (uint256 amountOut) {
        ..
        isWhiteListed(path[i].pool);

function exactInputSingleWithNativeToken(ExactInputSingleParams calldata params) public payable returns (uint256 amountOut) {
   .. // No isWhiteListed

function exactInputWithNativeToken(ExactInputParams calldata params) public payable returns (uint256 amountOut) {
        ..
        isWhiteListed(params.path[i].pool);

function complexPath(ComplexPathParams calldata params) public payable {
        ...
        isWhiteListed(params.initialPath[i].pool);
        ...
        isWhiteListed(params.percentagePath[i].pool);

Tools Used

Recommended Mitigation Steps

Check if the following functions also need isWhiteListed(..) to whitelist the pool. If so, add the isWhiteListed() statement:

maxsam4 commented 3 years ago

This is an intentional design choice. There are code comments in the contract that say "@/dev Ensure that the pool is trusted before calling this function. The pool can steal users' tokens.".

The reason this design was taken is that using exactInput and similar functions, an attacker can steal all user tokens in a single transaction (all tokens - usdt, dai, weth etc). Therefore, we added an extra check to save users from themselves.

On the other hand, exactInputSingle and similar functions only allow theft of one token in the worst case. This is the same as just transferring the token to a wrong address. Therefore, for gas efficiency, we omitted the check.

alcueca commented 3 years ago

As a design choice previously disclosed (through a code comment), this is not a previously unknown vulnerability.

loudoguno commented 2 years ago

replacing severity label with invalid as per judges findings sheet