Salty.IO - AgileJune's results

Lines of code

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

Vulnerability details

Vulnerability Detail

When reserve1 of a pool is equal to zero, any call to addLiquidity will cause the pool to reset the reserves ratio. (https://github.com/code-423n4/2024-01-salty/blob/main/src/pools/Pools.sol#L97) Zero of reserve1 can be done since of this missed checking about reserves.reserve1 (https://github.com/code-423n4/2024-01-salty/blob/main/src/pools/Pools.sol#L187) So even with a properly set slippage guard, a depositor calling addLiquidity can be front run by a sole liquidity holder, which would cause the depositor to lose funds.

Exploit Scenario

(It's just from Tail of bits's No. 3 except for bringing down reserve1 to zero)

Eve is the sole liquidity holder in the WETH/DAI pool, which has reserves of 100 WETH and 200,000 DAI, for a ratio of 1:2,000.

1. Alice submits a transaction to add 10 WETH and 20,000 DAI of liquidity.
2. Eve front runs Alice’s transaction with a removeLiquidity transaction that brings the reserve1 down to zero.
3. As the last action of her transaction, Eve adds liquidity, but because reserve1 is equal to zero, whatever ratio she adds to the pool becomes the new reserves ratio. In this example, Eve adds the ratio 10:2,000 (representing a WETH price of 200 DAI).
4. Alice’s addLiquidity transaction goes through, but because of the new K ratio, the logic lets her add only 10 WETH and 2,000 DAI. The liquidity slippage guard does not work because the reserves ratio has been reset. In nominal terms, Alice actually receives more liquidity than she would have at the previous ratio.
5. Eve back runs this transaction with a swap transaction that buys most of the WETH that Alice just deposited for a starting price of 200 DAI. Eve then removes her liquidity, effectively stealing Alice’s 10 WETH for a fraction of the price.

Tools Used

Manual Review

Recommended Mitigation Steps

We need to fix the condition correctly. https://github.com/code-423n4/2024-01-salty/blob/main/src/pools/Pools.sol#L187

before 
require((reserves.reserve0 >= PoolUtils.DUST) && (reserves.reserve0 >= PoolUtils.DUST), "Insufficient reserves after liquidity removal");

after
 require((reserves.reserve0 >= PoolUtils.DUST) && (reserves.reserve1 >= PoolUtils.DUST), "Insufficient reserves after liquidity removal");

Assessed type

Invalid Validation

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