search

code-423n4 / 2024-01-salty-findings

11 stars 6 forks source link

Token value in ETH calculated for arbitrage is computed wrong #704

Closed c4-bot-2 closed 9 months ago

c4-bot-2 commented 9 months ago

Lines of code

https://github.com/code-423n4/2024-01-salty/blob/main/src/pools/Pools.sol#L347-L359 https://github.com/code-423n4/2024-01-salty/blob/main/src/pools/Pools.sol#L235-L276

Vulnerability details

Impact

When a swap is executed, in order to set boundries for the arbitrage binary search, the value of the amountIn is calculated in terms of ETH by using pool reserves. This binary search will look for the most efficient amount to swap in order to get more profit. This amount will be bounded between amountIn // 128 and amountIn * 1,25.

The value calculation of the input amount is done using the pool reserves

    function _determineSwapAmountInValueInETH(IERC20 swapTokenIn, uint256 swapAmountIn) internal view returns (uint256 swapAmountInValueInETH)
        {
        if ( address(swapTokenIn) == address(weth) )
            return swapAmountIn;

        // All tokens within the DEX are paired with WETH (and WBTC) - so this pool will exist.
        (uint256 reservesWETH, uint256 reservesTokenIn) = getPoolReserves(weth, swapTokenIn);

        if ( (reservesWETH < PoolUtils.DUST) || (reservesTokenIn < PoolUtils.DUST) )
            return 0; // can't arbitrage as there are not enough reserves to determine swapAmountInValueInETH

        swapAmountInValueInETH = ( swapAmountIn * reservesWETH ) / reservesTokenIn;

        if ( swapAmountInValueInETH <= PoolUtils.DUST )
            return 0;
        }

However, this calculation is made right after the swap is accounted in the reserves:

    function _adjustReservesForSwapAndAttemptArbitrage( IERC20 swapTokenIn, IERC20 swapTokenOut, uint256 swapAmountIn, uint256 minAmountOut ) internal returns (uint256 swapAmountOut)
        {
        // Place the user swap first
@>      swapAmountOut = _adjustReservesForSwap( swapTokenIn, swapTokenOut, swapAmountIn );      // Reserves for the pool are updated here when the swap is computed

        // Make sure the swap meets the minimums specified by the user
        require( swapAmountOut >= minAmountOut, "Insufficient resulting token amount" );

        // The user's swap has just been made - attempt atomic arbitrage to rebalance the pool and yield arbitrage profit
@>      uint256 arbitrageProfit = _attemptArbitrage( swapTokenIn, swapTokenOut, swapAmountIn );         // Value of the input amount is computed here after the reserves has been updated

        emit SwapAndArbitrage(msg.sender, swapTokenIn, swapTokenOut, swapAmountIn, swapAmountOut, arbitrageProfit);
        }

As a result, when any token is swapped for ETH, the value of the input token will be computed with the already changed reserves of the pool which has suffered from the impact of the swap and the possible arbitrage profits for that pool will drop.

Note that the greater the swapped amount is, the less arbitrage profits will be obtained by the pool because the swap will impact significantly in its reserves. And the bigger amounts in the pool reserves, the less impact these kind of swaps will have.

Proof of Concept

Check this foundry test

Setup:

contract TestComprehensive1 is Deployment
    {
    // User wallets for testing
    address public constant alice = address(0x1111);
    address public constant bob = address(0x2222);
    address public constant charlie = address(0x3333);
    address public constant delta = address(0x4444);

    function setUp() public
        {
        initializeContracts();

        grantAccessAlice();
        grantAccessBob();
        grantAccessCharlie();
        grantAccessDeployer();
        grantAccessDefault();

        // Give some WBTC and WETH to Alice, Bob and Charlie
        vm.startPrank(DEPLOYER);
        wbtc.transfer(alice, 1000 * 10**8 );
        wbtc.transfer(bob, 1000 * 10**8 );
        wbtc.transfer(charlie, 1000 * 10**8 );

        weth.transfer(alice, 1000 ether);
        weth.transfer(bob, 1000 ether);
        weth.transfer(charlie, 1000 ether);
        vm.stopPrank();

        // Everyone approves
        vm.startPrank(alice);
        salt.approve(address(pools), type(uint256).max);
        wbtc.approve(address(pools), type(uint256).max);
        weth.approve(address(pools), type(uint256).max);
        salt.approve(address(collateralAndLiquidity), type(uint256).max);
        wbtc.approve(address(collateralAndLiquidity), type(uint256).max);
        weth.approve(address(collateralAndLiquidity), type(uint256).max);
        vm.stopPrank();

        vm.startPrank(bob);
        salt.approve(address(pools), type(uint256).max);
        wbtc.approve(address(pools), type(uint256).max);
        weth.approve(address(pools), type(uint256).max);
        salt.approve(address(collateralAndLiquidity), type(uint256).max);
        wbtc.approve(address(collateralAndLiquidity), type(uint256).max);
        weth.approve(address(collateralAndLiquidity), type(uint256).max);
        vm.stopPrank();

        vm.startPrank(charlie);
        salt.approve(address(pools), type(uint256).max);
        wbtc.approve(address(pools), type(uint256).max);
        weth.approve(address(pools), type(uint256).max);
        salt.approve(address(collateralAndLiquidity), type(uint256).max);
        wbtc.approve(address(collateralAndLiquidity), type(uint256).max);
        weth.approve(address(collateralAndLiquidity), type(uint256).max);
        wbtc.approve(address(collateralAndLiquidity), type(uint256).max);
        weth.approve(address(collateralAndLiquidity), type(uint256).max);
        vm.stopPrank();

        // Cast votes for the BootstrapBallot so that the initialDistribution can happen
        bytes memory sig = abi.encodePacked(aliceVotingSignature);
        vm.startPrank(alice);
        bootstrapBallot.vote(true, sig);
        vm.stopPrank();

        sig = abi.encodePacked(bobVotingSignature);
        vm.startPrank(bob);
        bootstrapBallot.vote(true, sig);
        vm.stopPrank();

        sig = abi.encodePacked(charlieVotingSignature);
        vm.startPrank(charlie);
        bootstrapBallot.vote(false, sig);
        vm.stopPrank();

        // Finalize the ballot to distribute SALT to the protocol contracts and start up the exchange
        vm.warp( bootstrapBallot.completionTimestamp() );
        bootstrapBallot.finalizeBallot();

        // Have alice, bob and charlie claim their xSALT airdrop
        vm.prank(alice);
        airdrop.claimAirdrop();
        vm.prank(bob);
        airdrop.claimAirdrop();
        vm.prank(charlie);
        airdrop.claimAirdrop();

        // Wait a day so that alice, bob and charlie receive some SALT emissions for their xSALT
        vm.warp( block.timestamp + 1 days );
        upkeep.performUpkeep();

        bytes32[] memory poolIDs = new bytes32[](1);
        poolIDs[0] = PoolUtils.STAKED_SALT;

        vm.prank(alice);
        staking.claimAllRewards(poolIDs);
        vm.prank(bob);
        staking.claimAllRewards(poolIDs);
        vm.prank(charlie);
        staking.claimAllRewards(poolIDs);
        }

Proof of Concept:

    function testWrongAmountInValueCalculation() public {
        vm.startPrank(DEPLOYER);
        weth.approve(address(collateralAndLiquidity), 20_000 ether);
        wbtc.approve(address(collateralAndLiquidity), 1_000 * 10**8);

        // Real example 1ETH = 2000$, 1BTC=40000$       1BTC=20ETH
        collateralAndLiquidity.depositCollateralAndIncreaseShare(1_000 * 10**8, 20_000 ether, 0, block.timestamp, false);
        vm.stopPrank();

        // The user will swap 500 BTC for ETH
        uint256 amountInBTC = 500 * 10**8;
        (uint256 reservesWETHBefore, uint256 reservesWBTCBefore) = pools.getPoolReserves(weth, wbtc);
        uint256 valueInEthCalculatedBefore = ( amountInBTC * reservesWETHBefore ) / reservesWBTCBefore;
        console.log("Real BTC value in terms of ETH", valueInEthCalculatedBefore);

        uint256 amountOutETH = amountInBTC * reservesWETHBefore / (reservesWBTCBefore + amountInBTC);
        uint256 reserveWETHAfter = reservesWETHBefore - amountOutETH;
        uint256 reserveBTCAfter = reservesWBTCBefore + amountInBTC;
        uint256 valueInEthCalculatedAfter = ( amountInBTC * reserveWETHAfter ) / reserveBTCAfter;
        console.log("Calculated BTC value in terms of ETH (used for arbitrage)", valueInEthCalculatedAfter);

        console.log("Value lost due to calculation with wrong reserves", valueInEthCalculatedBefore - valueInEthCalculatedAfter);
    }

Output traces:

Running 1 test for src/scenario_tests/Comprehensive1.t.sol:TestComprehensive1
[PASS] testWrongAmountInValueCalculation() (gas: 315392)
Logs:
  Real BTC value in terms of ETH 10000000000000000000000
  Calculated BTC value in terms of ETH (used for arbitrage) 4444444444444444444444
  Value lost due to calculation with wrong reserves 5555555555555555555556

Traces:
  [319604] TestComprehensive1::testWrongAmountInValueCalculation() 
    ├─ [0] VM::startPrank(0x73107dA86708c2DAd0D91388fB057EeE3E2581aF) 
    │   └─ ← ()
    ├─ [24603] TestERC20::approve(CollateralAndLiquidity: [0x933410f4a1fe18bb4c5ae1F01E9FC6f8dD0B6d4C], 20000000000000000000000 [2e22]) 
    │   ├─ emit Approval(owner: 0x73107dA86708c2DAd0D91388fB057EeE3E2581aF, spender: CollateralAndLiquidity: [0x933410f4a1fe18bb4c5ae1F01E9FC6f8dD0B6d4C], value: 20000000000000000000000 [2e22])
    │   └─ ← true
    ├─ [24603] TestERC20::approve(CollateralAndLiquidity: [0x933410f4a1fe18bb4c5ae1F01E9FC6f8dD0B6d4C], 100000000000 [1e11]) 
    │   ├─ emit Approval(owner: 0x73107dA86708c2DAd0D91388fB057EeE3E2581aF, spender: CollateralAndLiquidity: [0x933410f4a1fe18bb4c5ae1F01E9FC6f8dD0B6d4C], value: 100000000000 [1e11])
    │   └─ ← true
    ├─ [257619] CollateralAndLiquidity::depositCollateralAndIncreaseShare(100000000000 [1e11], 20000000000000000000000 [2e22], 0, 1706825892 [1.706e9], false) 
    │   ├─ [14386] ExchangeConfig::walletHasAccess(0x73107dA86708c2DAd0D91388fB057EeE3E2581aF) [staticcall]
    │   │   ├─ [4706] AccessManager::walletHasAccess(0x73107dA86708c2DAd0D91388fB057EeE3E2581aF) [staticcall]
    │   │   │   └─ ← true
    │   │   └─ ← true
    │   ├─ [25847] TestERC20::transferFrom(0x73107dA86708c2DAd0D91388fB057EeE3E2581aF, CollateralAndLiquidity: [0x933410f4a1fe18bb4c5ae1F01E9FC6f8dD0B6d4C], 100000000000 [1e11]) 
    │   │   ├─ emit Approval(owner: 0x73107dA86708c2DAd0D91388fB057EeE3E2581aF, spender: CollateralAndLiquidity: [0x933410f4a1fe18bb4c5ae1F01E9FC6f8dD0B6d4C], value: 0)
    │   │   ├─ emit Transfer(from: 0x73107dA86708c2DAd0D91388fB057EeE3E2581aF, to: CollateralAndLiquidity: [0x933410f4a1fe18bb4c5ae1F01E9FC6f8dD0B6d4C], value: 100000000000 [1e11])
    │   │   └─ ← true
    │   ├─ [25847] TestERC20::transferFrom(0x73107dA86708c2DAd0D91388fB057EeE3E2581aF, CollateralAndLiquidity: [0x933410f4a1fe18bb4c5ae1F01E9FC6f8dD0B6d4C], 20000000000000000000000 [2e22]) 
    │   │   ├─ emit Approval(owner: 0x73107dA86708c2DAd0D91388fB057EeE3E2581aF, spender: CollateralAndLiquidity: [0x933410f4a1fe18bb4c5ae1F01E9FC6f8dD0B6d4C], value: 0)
    │   │   ├─ emit Transfer(from: 0x73107dA86708c2DAd0D91388fB057EeE3E2581aF, to: CollateralAndLiquidity: [0x933410f4a1fe18bb4c5ae1F01E9FC6f8dD0B6d4C], value: 20000000000000000000000 [2e22])
    │   │   └─ ← true
    │   ├─ [24603] TestERC20::approve(Pools: [0x9230890252C028b4a45A40F460c721a6AAD4763e], 100000000000 [1e11]) 
    │   │   ├─ emit Approval(owner: CollateralAndLiquidity: [0x933410f4a1fe18bb4c5ae1F01E9FC6f8dD0B6d4C], spender: Pools: [0x9230890252C028b4a45A40F460c721a6AAD4763e], value: 100000000000 [1e11])
    │   │   └─ ← true
    │   ├─ [24603] TestERC20::approve(Pools: [0x9230890252C028b4a45A40F460c721a6AAD4763e], 20000000000000000000000 [2e22]) 
    │   │   ├─ emit Approval(owner: CollateralAndLiquidity: [0x933410f4a1fe18bb4c5ae1F01E9FC6f8dD0B6d4C], spender: Pools: [0x9230890252C028b4a45A40F460c721a6AAD4763e], value: 20000000000000000000000 [2e22])
    │   │   └─ ← true
    │   ├─ [73888] Pools::addLiquidity(TestERC20: [0xd47A53557a81B3A98855e6417F43F1df9a642a98], TestERC20: [0xaaEf7d54395fc25960E06720401f0C68280E25b2], 100000000000 [1e11], 20000000000000000000000 [2e22], 0, 0) 
    │   │   ├─ [22007] TestERC20::transferFrom(CollateralAndLiquidity: [0x933410f4a1fe18bb4c5ae1F01E9FC6f8dD0B6d4C], Pools: [0x9230890252C028b4a45A40F460c721a6AAD4763e], 100000000000 [1e11]) 
    │   │   │   ├─ emit Approval(owner: CollateralAndLiquidity: [0x933410f4a1fe18bb4c5ae1F01E9FC6f8dD0B6d4C], spender: Pools: [0x9230890252C028b4a45A40F460c721a6AAD4763e], value: 0)
    │   │   │   ├─ emit Transfer(from: CollateralAndLiquidity: [0x933410f4a1fe18bb4c5ae1F01E9FC6f8dD0B6d4C], to: Pools: [0x9230890252C028b4a45A40F460c721a6AAD4763e], value: 100000000000 [1e11])
    │   │   │   └─ ← true
    │   │   ├─ [22007] TestERC20::transferFrom(CollateralAndLiquidity: [0x933410f4a1fe18bb4c5ae1F01E9FC6f8dD0B6d4C], Pools: [0x9230890252C028b4a45A40F460c721a6AAD4763e], 20000000000000000000000 [2e22]) 
    │   │   │   ├─ emit Approval(owner: CollateralAndLiquidity: [0x933410f4a1fe18bb4c5ae1F01E9FC6f8dD0B6d4C], spender: Pools: [0x9230890252C028b4a45A40F460c721a6AAD4763e], value: 0)
    │   │   │   ├─ emit Transfer(from: CollateralAndLiquidity: [0x933410f4a1fe18bb4c5ae1F01E9FC6f8dD0B6d4C], to: Pools: [0x9230890252C028b4a45A40F460c721a6AAD4763e], value: 20000000000000000000000 [2e22])
    │   │   │   └─ ← true
    │   │   ├─ emit LiquidityAdded(tokenA: TestERC20: [0xd47A53557a81B3A98855e6417F43F1df9a642a98], tokenB: TestERC20: [0xaaEf7d54395fc25960E06720401f0C68280E25b2], addedAmountA: 100000000000 [1e11], addedAmountB: 20000000000000000000000 [2e22], addedLiquidity: 20000000000100000000000 [2e22])
    │   │   └─ ← 100000000000 [1e11], 20000000000000000000000 [2e22], 20000000000100000000000 [2e22]
    │   ├─ [2510] PoolsConfig::isWhitelisted(0xb3c31de4eb268467070b1c55e9f4424b08f0ad6036d6a0b77a2a5b838ac78b55) [staticcall]
    │   │   └─ ← true
    │   ├─ [370] ExchangeConfig::dao() [staticcall]
    │   │   └─ ← DAO: [0xAE65Fd081a1aAf0199d46D6101263489CcE9bd99]
    │   ├─ [2328] 0x01a1B171D93B7c5e0dF57a10465D905cb32Bb4C7::9852e419() [staticcall]
    │   │   └─ ← 0x0000000000000000000000000000000000000000000000000000000000000e10
    │   ├─ emit UserShareIncreased(wallet: 0x73107dA86708c2DAd0D91388fB057EeE3E2581aF, poolID: 0xb3c31de4eb268467070b1c55e9f4424b08f0ad6036d6a0b77a2a5b838ac78b55, amountIncreased: 20000000000100000000000 [2e22])
    │   ├─ emit LiquidityDeposited(user: 0x73107dA86708c2DAd0D91388fB057EeE3E2581aF, tokenA: TestERC20: [0xd47A53557a81B3A98855e6417F43F1df9a642a98], tokenB: TestERC20: [0xaaEf7d54395fc25960E06720401f0C68280E25b2], amountA: 100000000000 [1e11], amountB: 20000000000000000000000 [2e22], addedLiquidity: 20000000000100000000000 [2e22])
    │   ├─ emit CollateralDeposited(depositor: 0x73107dA86708c2DAd0D91388fB057EeE3E2581aF, amountWBTC: 100000000000 [1e11], amountWETH: 20000000000000000000000 [2e22], liquidity: 20000000000100000000000 [2e22])
    │   └─ ← 100000000000 [1e11], 20000000000000000000000 [2e22], 20000000000100000000000 [2e22]
    ├─ [0] VM::stopPrank() 
    │   └─ ← ()
    ├─ [1178] Pools::getPoolReserves(TestERC20: [0xaaEf7d54395fc25960E06720401f0C68280E25b2], TestERC20: [0xd47A53557a81B3A98855e6417F43F1df9a642a98]) [staticcall]
    │   └─ ← 20000000000000000000000 [2e22], 100000000000 [1e11]
    ├─ [0] console::9710a9d0(000000000000000000000000000000000000000000000000000000000000004000000000000000000000000000000000000000000000021e19e0c9bab2400000000000000000000000000000000000000000000000000000000000000000001e5265616c204254432076616c756520696e207465726d73206f66204554480000) [staticcall]
    │   └─ ← ()
    ├─ [0] console::9710a9d0(00000000000000000000000000000000000000000000000000000000000000400000000000000000000000000000000000000000000000f0ef0e928bdd71c71c000000000000000000000000000000000000000000000000000000000000003943616c63756c61746564204254432076616c756520696e207465726d73206f662045544820287573656420666f72206172626974726167652900000000000000) [staticcall]
    │   └─ ← ()
    ├─ [0] console::9710a9d0(000000000000000000000000000000000000000000000000000000000000004000000000000000000000000000000000000000000000012d2ad2372ed4ce38e4000000000000000000000000000000000000000000000000000000000000003156616c7565206c6f73742064756520746f2063616c63756c6174696f6e20776974682077726f6e67207265736572766573000000000000000000000000000000) [staticcall]
    │   └─ ← ()
    └─ ← ()

Test result: ok. 1 passed; 0 failed; finished in 10.27s

Notice that this value lost due to the calculation will leave open arbitrage oportunities to anyone and basically the protocol will not benefit properly from the main feature of this codebase.

Tools Used

Manual review

Recommended Mitigation Steps

Cache the reserves before the swap is executed and calculate the input value with these reserves previous to the swap

Assessed type

Error

c4-judge commented 9 months ago

Picodes marked the issue as unsatisfactory: Invalid

Picodes commented 9 months ago

It seems to work as intended as you want to do the arbitrage after the swap has been made?

Draiakoo commented 8 months ago

Yeah, the arbitrage is executed after the swap, however, it is needed to compute the swapped value in terms of ETH to bound the maximum and minimum amount of WETH to execute this arbitrage. To compute this value it is used the reserves of the pool involving the swapped token and WETH:

    function _attemptArbitrage( IERC20 swapTokenIn, IERC20 swapTokenOut, uint256 swapAmountIn ) internal returns (uint256 arbitrageProfit )
        {
        // Determine the value of swapTokenIn (in WETH) that the user has specified as it will impact the arbitrage trade size
        uint256 swapAmountInValueInETH = _determineSwapAmountInValueInETH(swapTokenIn, swapAmountIn);
        if ( swapAmountInValueInETH == 0 )
            return 0;

        ...
        }

    // Determine the WETH equivalent of swapAmountIn of swapTokenIn
    function _determineSwapAmountInValueInETH(IERC20 swapTokenIn, uint256 swapAmountIn) internal view returns (uint256 swapAmountInValueInETH)
        {
        if ( address(swapTokenIn) == address(weth) )
            return swapAmountIn;

        // All tokens within the DEX are paired with WETH (and WBTC) - so this pool will exist.
        (uint256 reservesWETH, uint256 reservesTokenIn) = getPoolReserves(weth, swapTokenIn);

        if ( (reservesWETH < PoolUtils.DUST) || (reservesTokenIn < PoolUtils.DUST) )
            return 0; // can't arbitrage as there are not enough reserves to determine swapAmountInValueInETH

@>      swapAmountInValueInETH = ( swapAmountIn * reservesWETH ) / reservesTokenIn;

        if ( swapAmountInValueInETH <= PoolUtils.DUST )
            return 0;
        }

Imagine that someone swaps USDS for DAI, this swap will only change the reserves for USDS and DAI pool, as a result, when it computes the value of the swapped amount in ETH it will look at the USDS-WETH reserves. Since these reserves have not been changed, the computation of the value will be correct.

However, when someone swaps any arbitrary token for WETH, it will always increase the reserve of this arbitrary token and will decrease the reserve of WETH. After changing these reserve amounts due to the swap, it will compute the value of the amount swapped in terms of ETH by calculating this:

                                 swapAmountIn * reservesWETH
swapAmountInValueInETH =----------------------------------------
                                    reservesTokenIn

Since the reservesWETH will have decreased and reservesTokenIn will have increased, the computed value will be much lower, and as a result, the arbitrage oportunity will not be fully leveraged by the protocol.

Example: The pool for WETH-BTC is balanced with the open market Initial reserves: WETH = 20.000 ETH BTC = 1.000 BTC

Now imagine that a user swaps 500 BTC for ETH, when a swap would be executed, the value of btc swapped in terms of ETH would need to be 500 BTC * 20 ETH = 10000 ETH according to the open market. However, since to calculate the value of the swapped amount in terms of ETH is based on the reserves of the token-WETH pool AFTER THE SWAP it will be wrongly calculated because the reserves will have changed. For this specific example:

After swapping 500 BTC the reserves remain as follows: WETH = 6666.7 ETH BTC = 1500 BTC

The value of the swapped amount would be computed as:

                                 500 * 6666.7
swapAmountInValueInETH =----------------------------- = 2222.2
                                    1500

As a result, the computed value to execute the arbitrage is 2222.2 WETH when in fact, the value swapped by the user was 10.000 WETH. Since the arbitrage is the main differentiator from other AMMs it really affects the correct functionality.

Picodes commented 8 months ago

@Draiakoo thanks.

Overall this report doesn't show precisely how this could be exploited and why it could be an issue. The most extreme case I see would be during a very large swap that the bisection search could be a bit misset but it seems very far fetched considering the user would lose a lot and the search should still find an arbitrage

Draiakoo commented 8 months ago

Hey @Picodes I wanted to say that it can not be exploited by a user, I was just refering that when a large swap is executed, the arbitrage will be done with less funds than it should. As a result, the protocol will get less arbitrage value (which is the main feature of this protocol) and it is also the way that liquidity providers get rewards. It is not critical but maybe a medium could be considered. Thanks for you patience!

Picodes commented 8 months ago

@Draiakoo I don't see how this leads to "the arbitrage will be done with less funds than it should".

What's happening is that the bisection search range can be lower than it should but in the extreme majority of the cases, the search range is large enough so a correct result will still be found.