c4-bot-1 commented 5 months ago

Lines of code

https://github.com/code-423n4/2024-01-salty/blob/main/src/staking/Liquidity.sol#L96-L97

Vulnerability details

Impact

Due to this vulnerability, users won't be able to deposit liquidity in pools with ERC-20 tokens that use approval race protection like USDT.

Proof of Concept

A user can deposit liquidity in a pool by calling Liquidity.sol depositLiquidityAndIncreaseShare() which then calls the internal function _depositLiquidityAndIncreaseShare().

In _depositLiquidityAndIncreaseShare() (only steps relevant to the bug are explained):

Both tokens of the reserve are approved for Pools contract so that they can be used by the pool for deposit.
An external call to Pools.addLiquidity() is made where:
1. The proportion of tokens to be transferred is calculated to maintain the reserve ratio
2. The calculated amount of tokens are transferred from the CollateralAndLiquidity contract to the Pools contract to add liquidity

We can see that the whole approved amount for either token might not be used by the Pools contract.

Given the scenario above, if the liquidity pool has a token with approval race protection like USDT, the deposits to the pool will revert if some amount is already approved to the Pools contract.

https://github.com/d-xo/weird-erc20#approval-race-protections

POC:

Pool -> tokenA=tokenA, tokenB=USDT
Alice calls depositLiquidityAndIncreaseShare() with maxAmountA=1000 and maxAmountB=1000
The reserve ratio is such that tokenA=1000 and USDT=500 are deposited
1. 1000 amount of each tokens got approved for use by the Pools contract i.e tokenA.approve(pools,1000) and USDT.approve(pools,1000)
2. Pools contract transferred the required amount i.e 1000 and 500 tokens
3. 500 USDT are still approved for Pools after the transfer above
Bob calls depositLiquidityAndIncreaseShare() with maxAmountA=1000 and maxAmountB=1000
USDT.approve(pools,1000) is called during the deposit, however, the transaction will revert due to race protection in the USDT contract

// USDT
function approve(address _spender, uint _value) public onlyPayloadSize(2 * 32) {

  // To change the approve amount you first have to reduce the addresses`
  //  allowance to zero by calling `approve(_spender, 0)` if it is not
  //  already 0 to mitigate the race condition described here:
  //  https://github.com/ethereum/EIPs/issues/20#issuecomment-263524729
  require(!((_value != 0) && (allowed[msg.sender][_spender] != 0)));

  allowed[msg.sender][_spender] = _value;
  Approval(msg.sender, _spender, _value);
}

// Liquidity.sol

function _depositLiquidityAndIncreaseShare(IERC20 tokenA, IERC20 tokenB, uint256 maxAmountA, uint256 maxAmountB, uint256 minLiquidityReceived, bool useZapping) internal returns ( ... ) {
    .
    .
    // Approve the liquidity to add
    tokenA.approve( address(pools), maxAmountA );
    tokenB.approve( address(pools), maxAmountB );

    // Deposit the specified liquidity into the Pools contract
        // The added liquidity will be owned by this contract. (external call to Pools contract)
        bytes32 poolID = PoolUtils._poolID( tokenA, tokenB );
        (addedAmountA, addedAmountB, addedLiquidity) = pools.addLiquidity( tokenA, tokenB, maxAmountA, maxAmountB, minLiquidityReceived, totalShares[poolID]);
    .
    .
    // If any of the user's tokens were not used, then send them back
   if ( addedAmountA < maxAmountA )
   tokenA.safeTransfer( msg.sender, maxAmountA - addedAmountA );

   if ( addedAmountB < maxAmountB )
   tokenB.safeTransfer( msg.sender, maxAmountB - addedAmountB );
}

Tools Used

Manual review

Recommended Mitigation Steps

The _depositLiquidityAndIncreaseShare() function checks after the deposit for unused tokens of the user to send them back. We can add approve(0) for the token to mitigate this issue

https://github.com/code-423n4/2024-01-salty/blob/main/src/staking/Liquidity.sol#L109-L114

Current

        // If any of the user's tokens were not used, then send them back
        if ( addedAmountA < maxAmountA )
            tokenA.safeTransfer( msg.sender, maxAmountA - addedAmountA );

        if ( addedAmountB < maxAmountB )
            tokenB.safeTransfer( msg.sender, maxAmountB - addedAmountB );

After mitigation

        // If any of the user's tokens were not used, then send them back
    if (addedAmountA < maxAmountA) {
      tokenA.safeTransfer(msg.sender, maxAmountA - addedAmountA);
      tokenA.approve(address(pools), 0);
    }

    if (addedAmountB < maxAmountB) {
      tokenB.safeTransfer(msg.sender, maxAmountB - addedAmountB);
      tokenB.approve(address(pools), 0);
    }