c4-submissions commented 1 year ago

Lines of code

https://github.com/GoodEntry-io/ge/blob/c7c7de57902e11e66c8186d93c5bb511b53a45b8/contracts/helper/V3Proxy.sol#L134

Vulnerability details

Impact

swapTokensForExactTokens will transfer the entire balance of assetIn to msg.sender after swap, which allows anyone to steal funds stuck in the contract and conflicts with emergencyWithdraw

Proof of Concept

    function emergencyWithdraw(ERC20 token) onlyOwner external {   
        token.safeTransfer(msg.sender, token.balanceOf( address(this) ) );  // msg.sender has been Required to be owner
    }

    function swapTokensForExactTokens(uint amountOut, uint amountInMax, address[] calldata path, address to, uint deadline) external returns (uint[] memory amounts) {
        require(path.length == 2, "Direct swap only");
        require(msg.sender == to, "Swap to self only");
        ERC20 ogInAsset = ERC20(path[0]);
        ogInAsset.safeTransferFrom(msg.sender, address(this), amountInMax);
        ogInAsset.safeApprove(address(ROUTER), amountInMax);
        amounts = new uint[](2);
        amounts[0] = ROUTER.exactOutputSingle(ISwapRouter.ExactOutputSingleParams(path[0], path[1], feeTier, msg.sender, deadline, amountOut, amountInMax, 0));         
        amounts[1] = amountOut; 
        ogInAsset.safeTransfer(msg.sender, ogInAsset.balanceOf(address(this)));
        ogInAsset.safeApprove(address(ROUTER), 0);
        emit Swap(msg.sender, path[0], path[1], amounts[0], amounts[1]); 
    }

    function swapTokensForExactETH(uint amountOut, uint amountInMax, address[] calldata path, address to, uint deadline) payable external returns (uint[] memory amounts) {
        require(path.length == 2, "Direct swap only");
        require(msg.sender == to, "Swap to self only");
        require(path[1] == ROUTER.WETH9(), "Invalid path");
        ERC20 ogInAsset = ERC20(path[0]);
        ogInAsset.safeTransferFrom(msg.sender, address(this), amountInMax);
        ogInAsset.safeApprove(address(ROUTER), amountInMax);
        amounts = new uint[](2);
        amounts[0] = ROUTER.exactOutputSingle(ISwapRouter.ExactOutputSingleParams(path[0], path[1], feeTier, address(this), deadline, amountOut, amountInMax, 0));         
        amounts[1] = amountOut; 
        ogInAsset.safeTransfer(msg.sender, amountInMax - amounts[0]);
        ogInAsset.safeApprove(address(ROUTER), 0);
        IWETH9 weth = IWETH9(ROUTER.WETH9());
        acceptPayable = true;
        weth.withdraw(amountOut);
        acceptPayable = false;
        payable(msg.sender).call{value: amountOut}("");
        emit Swap(msg.sender, path[0], path[1], amounts[0], amounts[1]); 
    }

swapTokensForExactTokens will transfer the entire balance of assetIn to msg.sender after swap. The correct approach should be like swapTokensForExactETH, only transfer amountInMax - amount[0].

Tools Used

Manual review

Recommended Mitigation Steps

Modify transfer amount

Assessed type

Context

gzeon-c4 commented 12 months ago

Low risk.

c4-judge commented 12 months ago

gzeon-c4 changed the severity to QA (Quality Assurance)