Potential Overflow Issue in Reserves Update due to Unsafe Casting

[c4-bot-4]

Lines of code

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

Vulnerability details

Impact

There is a potential risk of overflow during the update of liquidity pool reserves in Pools.sol _adjustReservesForSwap(). If the reserve of any token of a liquidity pool after swap is greater than type(uint128).max, then the type casting will result in an overflow.

reserves.reserve0 = uint128(reserve0);
reserves.reserve1 = uint128(reserve1);

The overflow may lead to unexpected and undesired behavior in the protocol, potentially affecting the reserves and reserve ratio of tokens in a liquidity pool which can be exploited by malicious actors.

The chance of this occuring is minimal but if it ever happens for a token, then the consequences will be disastrous so it's better to be cautious and take proactive measures.

Proof of Concept

The function Pools.sol _adjustReservesForSwap() is called during every swap and arbitrage. It temporarily stores reserves of pool in uint256 variables reserve0 and reserve1 for calculations.

The value of reserve0 and reserve1 is casted to uint128 to update the final reserves, reserves.reserve0 and reserves.reserve1, as shown below. As we can see, there exists a mismatch in the variable types.

E.g. if reserve0 > type(uint128).max, then reserves.reserve0 = uint128(reserve0) will result in an overflow.

function _adjustReservesForSwap(...) internal returns (...) {
    PoolReserves storage reserves = _poolReserves[poolID];
    uint256 reserve0 = reserves.reserve0;
    uint256 reserve1 = reserves.reserve1;
    .
    // swap logic
    .
    // Update the reserves
    reserves.reserve0 = uint128(reserve0);
    reserves.reserve1 = uint128(reserve1);
}

POC: The following test can be executed in Pools.t.sol

  function testAdjustReservesForSwap_Overflow_Bug_Audit() public {
    // setup
    vm.startPrank(alice);
    IERC20 tokenIn = new TestERC20('TEST', 18);
    IERC20 tokenOut = new TestERC20('TEST', 18);
    vm.stopPrank();

    vm.startPrank(address(dao));
    poolsConfig.whitelistPool(pools, tokenIn, tokenOut);
    vm.stopPrank();

    uint256 tokenInBalance = type(uint128).max - 1000;
    uint256 tokenOutBalance = type(uint128).max - 1000;

    deal(address(tokenIn), alice, type(uint256).max);
    deal(address(tokenOut), alice, type(uint256).max);

    // Add enough liquidity
    vm.startPrank(alice);
    tokenIn.transfer(address(collateralAndLiquidity), tokenInBalance);
    tokenOut.transfer(address(collateralAndLiquidity), tokenInBalance);
    vm.stopPrank();

    vm.startPrank(address(collateralAndLiquidity));
    tokenIn.approve(address(pools), type(uint256).max);
    tokenOut.approve(address(pools), type(uint256).max);
    uint256 totalShares = collateralAndLiquidity.totalShares(PoolUtils._poolID(tokenIn, tokenOut));

    pools.addLiquidity(tokenIn, tokenOut, tokenInBalance, tokenOutBalance, 0, totalShares);
    vm.stopPrank();

    // get pool reserves before swap
    (uint256 reserves0Before, uint256 reserves1Before) = pools.getPoolReserves(tokenIn, tokenOut);

    uint256 tokenAmountToSwap = 1500;
    uint256 minAmountOut = 1;

    vm.prank(alice);
    tokenIn.transfer(bob, tokenAmountToSwap + 100);

    uint256 tokenOutBalanceBeforeSwap = tokenOut.balanceOf(bob);
    vm.startPrank(bob);
    tokenIn.approve(address(pools), type(uint256).max);
    uint256 amountOut = pools.depositSwapWithdraw(
      tokenIn,
      tokenOut,
      tokenAmountToSwap,
      minAmountOut,
      block.timestamp + 1 minutes
    );
    (uint256 reserves0After, uint256 reserves1After) = pools.getPoolReserves(tokenIn, tokenOut);

    console.log('Reserves0 before swap: %s', reserves0Before);
    console.log('Reserves1 before swap: %s', reserves1Before);
    console.log('Reserves0 after swap: %s', reserves0After);
    console.log('Reserves1 after swap: %s', reserves1After);

    console.log('User tokenOut balance before swap: %s', tokenOutBalanceBeforeSwap);
    console.log('User tokenOut balance after 1st swap (tokenIn=1500): %s', tokenOut.balanceOf(bob));

    // swap with the new token reserve ratio due to uint128 overflow
    pools.depositSwapWithdraw(tokenIn, tokenOut, 100, minAmountOut, block.timestamp + 1 minutes);
    console.log('User tokenOut Balance after 2nd swap (tokenIn=100): %s', tokenOut.balanceOf(bob));
  }

Tools Used

Manual review

Recommended Mitigation Steps

Add a check, as shown in the code snippet below, to revert the transaction in case of an overflow.

// Update the reserves with overflow check
if (reserve0 > type(uint128).max || reserve1 > type(uint128).max) {
    // Handle or revert due to overflow
    revert("Overflow detected in reserves update");
}

reserves.reserve0 = uint128(reserve0);
reserves.reserve1 = uint128(reserve1);

Assessed type

Under/Overflow

[c4-judge]

Picodes marked the issue as primary issue

[c4-sponsor]

othernet-global (sponsor) confirmed

[othernet-global]

Updating reserves on swap now has overflow check.

https://github.com/othernet-global/salty-io/commit/a47139ba7a6fd5ccbbdef68e7ff964f4a411b533 https://github.com/othernet-global/salty-io/commit/f94310e4bf28c5965f10525059a7996564713427

[c4-judge]

Picodes marked the issue as satisfactory

[c4-judge]

Picodes marked the issue as selected for report

[fnanni-0]

Shouldn't this either be invalid or downgraded to low taking into account that:

1. Overflows caused by downcasting, including the lines mentioned in this issue, were pointed at in the automated findings report.
2. It's not possible to whitelist tokens with total supplies exceeding type(uint112).max (see proposeTokenWhitelisting).
3. This vulnerability would apparently happen in extremely unlikely scenarios. First we have to find a ERC20 token worth whitelisting with a total supply smaller than uint112 that would later be increased above a uint128. Also note that ERC20 with supplies bigger than uint128 are very rare. Looking at the top 1000 ERC20 tokens I couldn't find any. Second, SaltyIO DAO would have to approve a proposal whitelisting a token that would potentially not be supported by SaltyIO pools, maliciously or due to negligence.

[Picodes]

@fnanni-0 thanks for flagging. I do agree with your comment, especially concerning the fact that this should be out of scope.

[c4-judge]

Picodes marked the issue as unsatisfactory: Out of scope