The calling of function `depositLiquidityAndIncreaseShare()` with `useZapping = true` can be failed due to underflow on `_zapSwapAmount()`

[c4-bot-8]

Lines of code

https://github.com/code-423n4/2024-01-salty/blob/main/src/staking/Liquidity.sol#L146 https://github.com/code-423n4/2024-01-salty/blob/main/src/pools/PoolMath.sol#L178-L180 https://github.com/code-423n4/2024-01-salty/blob/main/src/pools/PoolMath.sol#L192

Vulnerability details

Summary

The calling of function depositLiquidityAndIncreaseShare() with useZapping = true can be failed due to underflow on _zapSwapAmount()

Vulnerability Detail

The calling of function depositLiquidityAndIncreaseShare() with useZapping = true invokes _zapSwapAmount() to calculate zap swap amount. BTW, after shifting, r1*z0 < r0*z1 is possible in two cases, so the line (https://github.com/code-423n4/2024-01-salty/blob/main/src/pools/PoolMath.sol#L192) causes underflow and reverts.

Case 1:

There are cases where z0 can be zero after shift. while passing the condition (https://github.com/code-423n4/2024-01-salty/blob/main/src/pools/PoolMath.sol#L178-L180) if shift variable is more than z0's bits count, it's possible

        function testDualZapInLiquidity() public {
            // setup
            TestERC20 tokenA = new TestERC20( "TEST", 18 );
            TestERC20 tokenB = new TestERC20( "TEST", 18 );

        uint256 zapAmountA = 2**80;
        uint256 zapAmountB = 2**91;

        vm.prank(address(dao));
        poolsConfig.whitelistPool( pools,   tokenA, tokenB);

            // make initial deposits
            tokenA.approve(address(collateralAndLiquidity),type(uint256).max);
            tokenB.approve(address(collateralAndLiquidity),type(uint256).max);
            collateralAndLiquidity.depositLiquidityAndIncreaseShare(tokenA, tokenB, zapAmountA, zapAmountB, 0, block.timestamp, false);
            vm.warp( block.timestamp + 1 hours );

            // Add more liquidity
        vm.expectRevert(stdError.arithmeticError);
            collateralAndLiquidity.depositLiquidityAndIncreaseShare(tokenA, tokenB, 2**9, 2**18, 0, block.timestamp, true);

        vm.warp( block.timestamp + 1 hours );

            // get actual reserves
            (uint256 actualAddedAmountA, uint256 actualAddedAmountB) = pools.getPoolReserves(tokenA, tokenB);
        assertEq(zapAmountA, actualAddedAmountA);
            assertEq(zapAmountB, actualAddedAmountB);
            }

Case 2:

There are cases where r1*z0 > r0*z1, but after shifting. it becomes r1*z0 < r0*z1 We can get x, y, a, b from 2xy + x + y = 2ab to meet (2x + 1) * (2y + 1) > 2a * 2b, x* y < a * b

        function testDualZapInLiquidity() public {
            // setup
            TestERC20 tokenA = new TestERC20( "TEST", 18 );
            TestERC20 tokenB = new TestERC20( "TEST", 18 );

            uint256 zapAmountA = 130478777887922532071812 * 2 * 2**15;
        uint256 zapAmountB = (2**81 + 1) * 2**10;

        vm.prank(address(dao));
        poolsConfig.whitelistPool( pools,   tokenA, tokenB);

            // make initial deposits
            tokenA.approve(address(collateralAndLiquidity),type(uint256).max);
            tokenB.approve(address(collateralAndLiquidity),type(uint256).max);
            collateralAndLiquidity.depositLiquidityAndIncreaseShare(tokenA, tokenB, zapAmountA, zapAmountB, 0, block.timestamp, false);
            vm.warp( block.timestamp + 1 hours );

        // Add more liquidity
        vm.expectRevert(stdError.arithmeticError);
            collateralAndLiquidity.depositLiquidityAndIncreaseShare(tokenA, tokenB, 49 * 2**10, 454 * 2**10,  0, block.timestamp, true);

            // get actual reserves
            (uint256 actualAddedAmountA, uint256 actualAddedAmountB) = pools.getPoolReserves(tokenA, tokenB);
            assertEq(zapAmountA, actualAddedAmountA);
            assertEq(zapAmountB, actualAddedAmountB);
            }

Code Snippet

https://github.com/code-423n4/2024-01-salty/blob/main/src/staking/Liquidity.sol#L146 https://github.com/code-423n4/2024-01-salty/blob/main/src/pools/PoolMath.sol#L178-L180 https://github.com/code-423n4/2024-01-salty/blob/main/src/pools/PoolMath.sol#L192

Tool used

Manual Review, Foundry

Recommendation

1. we can add a condition simply

   if (r1 * z0 < r0 * z1) {
   return 0;
   }
   uint256 C = r0 * ( r1 * z0 - r0 * z1 ) / ( r1 + z1 ); 

2. We can set criteria dynamically rather than current fixed 80, to avoid two edge cases explained above.

Assessed type

Under/Overflow

[c4-judge]

Picodes marked the issue as duplicate of #707

[c4-judge]

Picodes marked the issue as satisfactory

[c4-judge]

Picodes marked the issue as duplicate of #232

[c4-judge]

Picodes changed the severity to 2 (Med Risk)