sherlock-audit / 2022-10-mover-judging

1 stars 0 forks source link

Chom - Missing slippage control. If swap using another protocol that doesn't support slippage control, users may lose all fund to MEV bots #53

Closed sherlock-admin closed 2 years ago

sherlock-admin commented 2 years ago

Chom

high

Missing slippage control. If swap using another protocol that doesn't support slippage control, users may lose all fund to MEV bots

Summary

Missing slippage control. If swap using another protocol that doesn't support slippage control, users may lose all funds to MEV bots.

Vulnerability Detail

 function executeSwapDirect( 
     address _beneficiary, 
     address _tokenFrom, 
     address _tokenTo, 
     uint256 _amount, 
     uint256 _exchangeFee, 
     bytes memory _data 
 )

No minimum output amount in executeSwapDirect function parameters

        // ensure no state passed, no reentrancy, etc.
        (bool success, ) = executorAddress.call{value: ethValue}(callData);
        require(success, "SWAP_CALL_FAILED");

This call an external swap. For example, calling swap on the new Quickswap recently audited by code4rena https://github.com/code-423n4/2022-09-quickswap. Assuming Quickswap refuses to mitigate the slippage control issue, calling a swap on Quickswap won't have any slippage protection. MEV bots can use this opportunity to steal nearly all funds through the sandwich attack.

Developers may not be noticed this vulnerability while whitelisting executorAddress.

Impact

Nearly all funds that are put into swapping in an exchange that doesn't have slippage protection can be stolen by MEV bots through the sandwich attack.

Code Snippet

https://github.com/sherlock-audit/2022-10-mover/blob/main/cardtopup_contract/contracts/ExchangeProxy.sol#L131-L210

Tool used

Manual Review

Recommendation

Add _minAmountOut to executeSwapDirect

 function executeSwapDirect( 
     address _beneficiary, 
     address _tokenFrom, 
     address _tokenTo, 
     uint256 _amount, 
     uint256 _minAmountOut,
     uint256 _exchangeFee, 
     bytes memory _data 
 )

Ensure that amountReceived is at least _minAmountOut

        // ensure no state passed, no reentrancy, etc.
        (bool success, ) = executorAddress.call{value: ethValue}(callData);
        require(success, "SWAP_CALL_FAILED");

        // always rely only on actual amount received regardless of called parameters
        uint256 amountReceived = 0;
        if (_tokenTo != ETH_TOKEN_ADDRESS) {
            amountReceived = IERC20(_tokenTo).balanceOf(address(this));
        } else {
            amountReceived = address(this).balance;
        }
        amountReceived = amountReceived.sub(balanceBefore);

        require(amountReceived > 0 && amountReceived >= _minAmountOut, "not enough output");
McMannaman commented 2 years ago

This is out of scope. MEV bots and swap efficiency is more related to swap aggregator. Exchange proxy is used only by Topup proxy and has minimum expected value check to serve as high-level sanity check of funds returned. Adding _minAmountOut would mean exactly same check. Both 0x and 1inch which are intended to be used have a degree of slippage control too.