search

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

11 stars 6 forks source link

Depositing liquidity with zapping can revert due to underflow #707

Closed c4-bot-6 closed 8 months ago

c4-bot-6 commented 9 months ago

Lines of code

https://github.com/code-423n4/2024-01-salty/blob/main/src/pools/PoolMath.sol#L192 https://github.com/code-423n4/2024-01-salty/blob/main/src/pools/PoolMath.sol#L212-L227 https://github.com/code-423n4/2024-01-salty/blob/main/src/pools/PoolMath.sol#L152-L207

Vulnerability details

Impact

When a user would try to add liquidity to a pool with zapping (having unbalanced amounts), he can get his transaction reverted due to an underflow in certain scenarios. User will not lose anything but a functionality of the system can get broken in some situations.

For a user to add liquidity with unbalanced amounts of assets, have to call the function depositLiquidityAndIncreaseShare enabling the useZapping feature. This will gonna compute the amount that needs to be swapped from one token to the other in order to have a balanced ratio of the tokens compared to the pool. This computation is done in this function:

    function _zapSwapAmount( uint256 reserve0, uint256 reserve1, uint256 zapAmount0, uint256 zapAmount1 ) internal pure returns (uint256 swapAmount)
        {
        uint256 maximumMSB = _maximumMSB( reserve0, reserve1, zapAmount0, zapAmount1);

        uint256 shift = 0;

        // Assumes the largest number has more than 80 bits - but if not then shifts zero effectively as a straight assignment.
        // C will be calculated as: C = r0 * ( r1 * z0 - r0 * z1 ) / ( r1 + z1 );
        // Multiplying three 80 bit numbers will yield 240 bits - within the 256 bit limit.
        if ( maximumMSB > 80 )
            shift = maximumMSB - 80;

        // Normalize the inputs to 80 bits.
        uint256 r0 = reserve0 >> shift;
        uint256 r1 = reserve1 >> shift;
        uint256 z0 = zapAmount0 >> shift;
        uint256 z1 = zapAmount1 >> shift;

        // In order to swap and zap, require that the reduced precision reserves and one of the zapAmounts exceed DUST.
        // Otherwise their value was too small and was crushed by the above precision reduction and we should just return swapAmounts of zero so that default addLiquidity will be attempted without a preceding swap.
        if ( r0 < PoolUtils.DUST)
            return 0;

        if ( r1 < PoolUtils.DUST)
            return 0;

        if ( z0 < PoolUtils.DUST)
        if ( z1 < PoolUtils.DUST)
            return 0;

        // Components of the quadratic formula mentioned in the initial comment block: x = [-B + sqrt(B^2 - 4AC)] / 2A
        uint256 A = 1;
        uint256 B = 2 * r0;

        // Here for reference
//        uint256 C = r0 * ( r0 * z1 - r1 * z0 ) / ( r1 + z1 );
//        uint256 discriminant = B * B - 4 * A * C;

        // Negate C (from above) and add instead of subtract.
        // r1 * z0 guaranteed to be greater than r0 * z1 per the conditional check in _determineZapSwapAmount
        uint256 C = r0 * ( r1 * z0 - r0 * z1 ) / ( r1 + z1 );
        uint256 discriminant = B * B + 4 * A * C;

        // Compute the square root of the discriminant.
        uint256 sqrtDiscriminant = Math.sqrt(discriminant);

        // Safety check: make sure B is not greater than sqrtDiscriminant
        if ( B > sqrtDiscriminant )
            return 0;

        // Only use the positive sqrt of the discriminant from: x = (-B +/- sqrtDiscriminant) / 2A
        swapAmount = ( sqrtDiscriminant - B ) / ( 2 * A );

        // Denormalize from the 80 bit representation
        swapAmount <<= shift;
        }

The only line that is capable of underflowing is this one:

uint256 C = r0 * ( r1 * z0 - r0 * z1 ) / ( r1 + z1 );

However, there is a comment above that tells us that it is checked before in a previous function.

Specifically here:

    function _determineZapSwapAmount( uint256 reserveA, uint256 reserveB, uint256 zapAmountA, uint256 zapAmountB ) internal pure returns (uint256 swapAmountA, uint256 swapAmountB )
        {
        // zapAmountA / zapAmountB exceeds the ratio of reserveA / reserveB? - meaning too much zapAmountA
        if ( zapAmountA * reserveB > reserveA * zapAmountB )
            (swapAmountA, swapAmountB) = (_zapSwapAmount( reserveA, reserveB, zapAmountA, zapAmountB ), 0);

        // zapAmountA / zapAmountB is less than the ratio of reserveA / reserveB? - meaning too much zapAmountB
        if ( zapAmountA * reserveB < reserveA * zapAmountB )
            (swapAmountA, swapAmountB) = (0, _zapSwapAmount( reserveB, reserveA, zapAmountB, zapAmountA ));

        // Ensure we are not swapping more than was specified for zapping
        if ( ( swapAmountA > zapAmountA ) || ( swapAmountB > zapAmountB ) )
            return (0, 0);

        return (swapAmountA, swapAmountB);
        }

We can see that this condition is checked in order to not underflow. However, since in the computation for the swap amount normalizes the different reserves and amounts to have less bits, if one of the reserves/amounts was greater than 0 and gets shifted to 0, this condition can reverse and it would result in an underflow.

To have a clear image, consider the following example:

reserveA = 20_000_000 ether;
reserveB = 1_000 * 10**6;
amountA = 0.01 ether;
amountB = 15;

The user wants to add 0.01 * 10**18 of the tokenA and have some dust amount of tokenB.

When it computes the condition in _determineZapSwapAmount function, it will achieve the following:

zapAmountA * reserveB < reserveA * zapAmountB
10000000000000000 * 1000000000 < 20000000000000000000000000 * 15

However, when these amounts and reserves are normalized, they get shifted 4 bits (just for this situation and for these amounts). So this condition will not be met with the normalized amounts:

625000000000000 * 62500000 > 1250000000000000000000000 * 0

Basically this will underflow and revert the whole transaction.

Notice that this just happens when one of the amounts in this condition is positive before normalization and gets 0 after bit shifting.

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 testUnderflowZapping() public {
        vm.startPrank(DEPLOYER);
        weth.approve(address(collateralAndLiquidity), 20_000_000 ether);
        wbtc.approve(address(collateralAndLiquidity), 1_000 * 10**6);

        collateralAndLiquidity.depositCollateralAndIncreaseShare(1_000 * 10**6, 20_000_000 ether, 0, block.timestamp, false);
        vm.stopPrank();

        vm.startPrank(alice);
        weth.approve(address(collateralAndLiquidity), 0.01 ether);
        wbtc.approve(address(collateralAndLiquidity), 15);
        vm.expectRevert();      // this revert due to underflow
        collateralAndLiquidity.depositCollateralAndIncreaseShare(15, 0.01 ether, 0, block.timestamp, true);
        vm.stopPrank();
    }

Output traces:

Running 1 test for src/scenario_tests/Comprehensive1.t.sol:TestComprehensive1
[PASS] testUnderflowZapping() (gas: 383356)
Traces:
  [387569] TestComprehensive1::testUnderflowZapping() 
    ├─ [0] VM::startPrank(0x73107dA86708c2DAd0D91388fB057EeE3E2581aF) 
    │   └─ ← ()
    ├─ [24603] TestERC20::approve(CollateralAndLiquidity: [0x933410f4a1fe18bb4c5ae1F01E9FC6f8dD0B6d4C], 20000000000000000000000000 [2e25]) 
    │   ├─ emit Approval(owner: 0x73107dA86708c2DAd0D91388fB057EeE3E2581aF, spender: CollateralAndLiquidity: [0x933410f4a1fe18bb4c5ae1F01E9FC6f8dD0B6d4C], value: 20000000000000000000000000 [2e25])
    │   └─ ← true
    ├─ [24603] TestERC20::approve(CollateralAndLiquidity: [0x933410f4a1fe18bb4c5ae1F01E9FC6f8dD0B6d4C], 1000000000 [1e9]) 
    │   ├─ emit Approval(owner: 0x73107dA86708c2DAd0D91388fB057EeE3E2581aF, spender: CollateralAndLiquidity: [0x933410f4a1fe18bb4c5ae1F01E9FC6f8dD0B6d4C], value: 1000000000 [1e9])
    │   └─ ← true
    ├─ [257619] CollateralAndLiquidity::depositCollateralAndIncreaseShare(1000000000 [1e9], 20000000000000000000000000 [2e25], 0, 1706828184 [1.706e9], false) 
    │   ├─ [14386] ExchangeConfig::walletHasAccess(0x73107dA86708c2DAd0D91388fB057EeE3E2581aF) [staticcall]
    │   │   ├─ [4706] AccessManager::walletHasAccess(0x73107dA86708c2DAd0D91388fB057EeE3E2581aF) [staticcall]
    │   │   │   └─ ← true
    │   │   └─ ← true
    │   ├─ [25847] TestERC20::transferFrom(0x73107dA86708c2DAd0D91388fB057EeE3E2581aF, CollateralAndLiquidity: [0x933410f4a1fe18bb4c5ae1F01E9FC6f8dD0B6d4C], 1000000000 [1e9]) 
    │   │   ├─ emit Approval(owner: 0x73107dA86708c2DAd0D91388fB057EeE3E2581aF, spender: CollateralAndLiquidity: [0x933410f4a1fe18bb4c5ae1F01E9FC6f8dD0B6d4C], value: 0)
    │   │   ├─ emit Transfer(from: 0x73107dA86708c2DAd0D91388fB057EeE3E2581aF, to: CollateralAndLiquidity: [0x933410f4a1fe18bb4c5ae1F01E9FC6f8dD0B6d4C], value: 1000000000 [1e9])
    │   │   └─ ← true
    │   ├─ [25847] TestERC20::transferFrom(0x73107dA86708c2DAd0D91388fB057EeE3E2581aF, CollateralAndLiquidity: [0x933410f4a1fe18bb4c5ae1F01E9FC6f8dD0B6d4C], 20000000000000000000000000 [2e25]) 
    │   │   ├─ emit Approval(owner: 0x73107dA86708c2DAd0D91388fB057EeE3E2581aF, spender: CollateralAndLiquidity: [0x933410f4a1fe18bb4c5ae1F01E9FC6f8dD0B6d4C], value: 0)
    │   │   ├─ emit Transfer(from: 0x73107dA86708c2DAd0D91388fB057EeE3E2581aF, to: CollateralAndLiquidity: [0x933410f4a1fe18bb4c5ae1F01E9FC6f8dD0B6d4C], value: 20000000000000000000000000 [2e25])
    │   │   └─ ← true
    │   ├─ [24603] TestERC20::approve(Pools: [0x9230890252C028b4a45A40F460c721a6AAD4763e], 1000000000 [1e9]) 
    │   │   ├─ emit Approval(owner: CollateralAndLiquidity: [0x933410f4a1fe18bb4c5ae1F01E9FC6f8dD0B6d4C], spender: Pools: [0x9230890252C028b4a45A40F460c721a6AAD4763e], value: 1000000000 [1e9])
    │   │   └─ ← true
    │   ├─ [24603] TestERC20::approve(Pools: [0x9230890252C028b4a45A40F460c721a6AAD4763e], 20000000000000000000000000 [2e25]) 
    │   │   ├─ emit Approval(owner: CollateralAndLiquidity: [0x933410f4a1fe18bb4c5ae1F01E9FC6f8dD0B6d4C], spender: Pools: [0x9230890252C028b4a45A40F460c721a6AAD4763e], value: 20000000000000000000000000 [2e25])
    │   │   └─ ← true
    │   ├─ [73888] Pools::addLiquidity(TestERC20: [0xd47A53557a81B3A98855e6417F43F1df9a642a98], TestERC20: [0xaaEf7d54395fc25960E06720401f0C68280E25b2], 1000000000 [1e9], 20000000000000000000000000 [2e25], 0, 0) 
    │   │   ├─ [22007] TestERC20::transferFrom(CollateralAndLiquidity: [0x933410f4a1fe18bb4c5ae1F01E9FC6f8dD0B6d4C], Pools: [0x9230890252C028b4a45A40F460c721a6AAD4763e], 1000000000 [1e9]) 
    │   │   │   ├─ emit Approval(owner: CollateralAndLiquidity: [0x933410f4a1fe18bb4c5ae1F01E9FC6f8dD0B6d4C], spender: Pools: [0x9230890252C028b4a45A40F460c721a6AAD4763e], value: 0)
    │   │   │   ├─ emit Transfer(from: CollateralAndLiquidity: [0x933410f4a1fe18bb4c5ae1F01E9FC6f8dD0B6d4C], to: Pools: [0x9230890252C028b4a45A40F460c721a6AAD4763e], value: 1000000000 [1e9])
    │   │   │   └─ ← true
    │   │   ├─ [22007] TestERC20::transferFrom(CollateralAndLiquidity: [0x933410f4a1fe18bb4c5ae1F01E9FC6f8dD0B6d4C], Pools: [0x9230890252C028b4a45A40F460c721a6AAD4763e], 20000000000000000000000000 [2e25]) 
    │   │   │   ├─ emit Approval(owner: CollateralAndLiquidity: [0x933410f4a1fe18bb4c5ae1F01E9FC6f8dD0B6d4C], spender: Pools: [0x9230890252C028b4a45A40F460c721a6AAD4763e], value: 0)
    │   │   │   ├─ emit Transfer(from: CollateralAndLiquidity: [0x933410f4a1fe18bb4c5ae1F01E9FC6f8dD0B6d4C], to: Pools: [0x9230890252C028b4a45A40F460c721a6AAD4763e], value: 20000000000000000000000000 [2e25])
    │   │   │   └─ ← true
    │   │   ├─ emit LiquidityAdded(tokenA: TestERC20: [0xd47A53557a81B3A98855e6417F43F1df9a642a98], tokenB: TestERC20: [0xaaEf7d54395fc25960E06720401f0C68280E25b2], addedAmountA: 1000000000 [1e9], addedAmountB: 20000000000000000000000000 [2e25], addedLiquidity: 20000000000000001000000000 [2e25])
    │   │   └─ ← 1000000000 [1e9], 20000000000000000000000000 [2e25], 20000000000000001000000000 [2e25]
    │   ├─ [2510] PoolsConfig::isWhitelisted(0xb3c31de4eb268467070b1c55e9f4424b08f0ad6036d6a0b77a2a5b838ac78b55) [staticcall]
    │   │   └─ ← true
    │   ├─ [370] ExchangeConfig::dao() [staticcall]
    │   │   └─ ← DAO: [0xAE65Fd081a1aAf0199d46D6101263489CcE9bd99]
    │   ├─ [2328] 0x01a1B171D93B7c5e0dF57a10465D905cb32Bb4C7::9852e419() [staticcall]
    │   │   └─ ← 0x0000000000000000000000000000000000000000000000000000000000000e10
    │   ├─ emit UserShareIncreased(wallet: 0x73107dA86708c2DAd0D91388fB057EeE3E2581aF, poolID: 0xb3c31de4eb268467070b1c55e9f4424b08f0ad6036d6a0b77a2a5b838ac78b55, amountIncreased: 20000000000000001000000000 [2e25])
    │   ├─ emit LiquidityDeposited(user: 0x73107dA86708c2DAd0D91388fB057EeE3E2581aF, tokenA: TestERC20: [0xd47A53557a81B3A98855e6417F43F1df9a642a98], tokenB: TestERC20: [0xaaEf7d54395fc25960E06720401f0C68280E25b2], amountA: 1000000000 [1e9], amountB: 20000000000000000000000000 [2e25], addedLiquidity: 20000000000000001000000000 [2e25])
    │   ├─ emit CollateralDeposited(depositor: 0x73107dA86708c2DAd0D91388fB057EeE3E2581aF, amountWBTC: 1000000000 [1e9], amountWETH: 20000000000000000000000000 [2e25], liquidity: 20000000000000001000000000 [2e25])
    │   └─ ← 1000000000 [1e9], 20000000000000000000000000 [2e25], 20000000000000001000000000 [2e25]
    ├─ [0] VM::stopPrank() 
    │   └─ ← ()
    ├─ [0] VM::startPrank(0x0000000000000000000000000000000000001111) 
    │   └─ ← ()
    ├─ [7503] TestERC20::approve(CollateralAndLiquidity: [0x933410f4a1fe18bb4c5ae1F01E9FC6f8dD0B6d4C], 10000000000000000 [1e16]) 
    │   ├─ emit Approval(owner: 0x0000000000000000000000000000000000001111, spender: CollateralAndLiquidity: [0x933410f4a1fe18bb4c5ae1F01E9FC6f8dD0B6d4C], value: 10000000000000000 [1e16])
    │   └─ ← true
    ├─ [7503] TestERC20::approve(CollateralAndLiquidity: [0x933410f4a1fe18bb4c5ae1F01E9FC6f8dD0B6d4C], 15) 
    │   ├─ emit Approval(owner: 0x0000000000000000000000000000000000001111, spender: CollateralAndLiquidity: [0x933410f4a1fe18bb4c5ae1F01E9FC6f8dD0B6d4C], value: 15)
    │   └─ ← true
    ├─ [0] VM::expectRevert() 
    │   └─ ← ()
    ├─ [66920] CollateralAndLiquidity::depositCollateralAndIncreaseShare(15, 10000000000000000 [1e16], 0, 1706828184 [1.706e9], true) 
    │   ├─ [3886] ExchangeConfig::walletHasAccess(0x0000000000000000000000000000000000001111) [staticcall]
    │   │   ├─ [2706] AccessManager::walletHasAccess(0x0000000000000000000000000000000000001111) [staticcall]
    │   │   │   └─ ← true
    │   │   └─ ← true
    │   ├─ [25508] TestERC20::transferFrom(0x0000000000000000000000000000000000001111, CollateralAndLiquidity: [0x933410f4a1fe18bb4c5ae1F01E9FC6f8dD0B6d4C], 15) 
    │   │   ├─ emit Approval(owner: 0x0000000000000000000000000000000000001111, spender: CollateralAndLiquidity: [0x933410f4a1fe18bb4c5ae1F01E9FC6f8dD0B6d4C], value: 0)
    │   │   ├─ emit Transfer(from: 0x0000000000000000000000000000000000001111, to: CollateralAndLiquidity: [0x933410f4a1fe18bb4c5ae1F01E9FC6f8dD0B6d4C], value: 15)
    │   │   └─ ← true
    │   ├─ [25508] TestERC20::transferFrom(0x0000000000000000000000000000000000001111, CollateralAndLiquidity: [0x933410f4a1fe18bb4c5ae1F01E9FC6f8dD0B6d4C], 10000000000000000 [1e16]) 
    │   │   ├─ emit Approval(owner: 0x0000000000000000000000000000000000001111, spender: CollateralAndLiquidity: [0x933410f4a1fe18bb4c5ae1F01E9FC6f8dD0B6d4C], value: 0)
    │   │   ├─ emit Transfer(from: 0x0000000000000000000000000000000000001111, to: CollateralAndLiquidity: [0x933410f4a1fe18bb4c5ae1F01E9FC6f8dD0B6d4C], value: 10000000000000000 [1e16])
    │   │   └─ ← true
    │   ├─ [1197] Pools::getPoolReserves(TestERC20: [0xd47A53557a81B3A98855e6417F43F1df9a642a98], TestERC20: [0xaaEf7d54395fc25960E06720401f0C68280E25b2]) [staticcall]
    │   │   └─ ← 1000000000 [1e9], 20000000000000000000000000 [2e25]
    │   └─ ← "Arithmetic over/underflow"
    ├─ [0] VM::stopPrank() 
    │   └─ ← ()
    └─ ← ()

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

Tools Used

Fuzzing

Recommended Mitigation Steps

Do not use normalization of the reserves and amounts because that is the root cause of this issue

Assessed type

Under/Overflow

c4-judge commented 9 months ago

Picodes marked the issue as primary issue

c4-sponsor commented 9 months ago

othernet-global (sponsor) confirmed

othernet-global commented 9 months ago

Fixed with: https://github.com/othernet-global/salty-io/commit/44320a8cc9b94de433e437e025f072aa850b995a

c4-judge commented 8 months ago

Picodes marked the issue as satisfactory

c4-judge commented 8 months ago

Picodes marked the issue as selected for report

Picodes commented 8 months ago

This report shows how in certain conditions depositLiquidityAndIncreaseShare with zapping reverts due to an underflow. Although the impact remains minimal this is a DoS.

c4-judge commented 8 months ago

Picodes marked the issue as duplicate of #232

c4-judge commented 8 months ago

Picodes marked the issue as not selected for report