Salty.IO - aman's results

Lines of code

https://github.com/code-423n4/2024-01-salty/blob/53516c2cdfdfacb662cdea6417c52f23c94d5b5b/src/stable/CollateralAndLiquidity.sol#L151 https://github.com/code-423n4/2024-01-salty/blob/53516c2cdfdfacb662cdea6417c52f23c94d5b5b/src/pools/Pools.sol#L187

Vulnerability details

Impact

The protocol expects that whenever the borrower's collateral falls below the allowed threshold for borrowed USDS, the user must be liquidated. However, it misses an edge case: when a user is liquidated, their liquidity is removed from the pool. If the reserves in the pool are less than the DUST threshold after this liquidity removal, it could lead to a systemic issue, causing bad debt in the system and rendering USDS undercollateralized.

function removeLiquidity( IERC20 tokenA, IERC20 tokenB, uint256 liquidityToRemove, uint256 minReclaimedA, uint256 minReclaimedB, uint256 totalLiquidity ) external nonReentrant returns (uint256 reclaimedA, uint256 reclaimedB)
		{
		require( msg.sender == address(collateralAndLiquidity), "Pools.removeLiquidity is only callable from the CollateralAndLiquidity contract" );
		require( liquidityToRemove > 0, "The amount of liquidityToRemove cannot be zero" );

		(bytes32 poolID, bool flipped) = PoolUtils._poolIDAndFlipped(tokenA, tokenB);

		// Determine how much liquidity is being withdrawn and round down in favor of the protocol
		PoolReserves storage reserves = _poolReserves[poolID];
		reclaimedA = ( reserves.reserve0 * liquidityToRemove ) / totalLiquidity;
		reclaimedB = ( reserves.reserve1 * liquidityToRemove ) / totalLiquidity;

		reserves.reserve0 -= uint128(reclaimedA);
		reserves.reserve1 -= uint128(reclaimedB);

		// Make sure that removing liquidity doesn't drive either of the reserves below DUST.
		// This is to ensure that ratios remain relatively constant even after a maximum withdrawal.
		// @audit-issue :  if amount left is less than Dust , it will revert, so user can not be liquidated until and less another or liquidator add liquidity into this pool
@>        require((reserves.reserve0 >= PoolUtils.DUST) && (reserves.reserve0 >= PoolUtils.DUST), "Insufficient reserves after liquidity removal");

		// Switch reclaimed amounts back to the order that was specified in the call arguments so they make sense to the caller
		if (flipped)
			(reclaimedA,reclaimedB) = (reclaimedB,reclaimedA);

		require( (reclaimedA >= minReclaimedA) && (reclaimedB >= minReclaimedB), "Insufficient underlying tokens returned" );

		// Send the reclaimed tokens to the user
		tokenA.safeTransfer( msg.sender, reclaimedA );
		tokenB.safeTransfer( msg.sender, reclaimedB );

		emit LiquidityRemoved(tokenA, tokenB, reclaimedA, reclaimedB, liquidityToRemove);
		}

Proof of Concept

Consider a situation where there is only one liquidity provider in the WBTC/WETH pool, and they borrowed the maxborrowableUSDS. After some time, the user becomes liquidatable. If someone attempts to liquidate the user, the function will result in a revert due to this check require((reserves.reserve0 >= PoolUtils.DUST) && (reserves.reserve0 >= PoolUtils.DUST), "Insufficient reserves after liquidity removal");.

The Flow of liquidation :

The liquidator calls CollateralAndLiquidity::liquidateUser function. This function call Pools::removeLiquidity with following arguments:

		(uint256 reclaimedWBTC, uint256 reclaimedWETH) = pools.removeLiquidity(wbtc, weth, userCollateralAmount, 0, 0, totalShares[collateralPoolID] , false);

Pools:removeLiquidity function checks for remaining reserves in Pool, after removing user liquidity/reserve. The Dos can happen here

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

I have assumed that the user is liquidatable in above case.

function testUserIsNotLiquidatable() public {
        // Alice attempts to deposit zero collateral which should not be allowed.
        vm.startPrank(alice);
        collateralAndLiquidity.depositCollateralAndIncreaseShare(wbtc.balanceOf(alice), PoolUtils.DUST + 1, 0, block.timestamp, false);
        uint256 maxUSDS = collateralAndLiquidity.maxBorrowableUSDS(alice);
        collateralAndLiquidity.borrowUSDS( maxUSDS );
        vm.stopPrank();

        _crashCollateralPrice();
        		vm.warp( block.timestamp + 1 day );

        assertEq(collateralAndLiquidity.canUserBeLiquidated(alice) , true);   
        // @audit : the user can not liquidated because the pool reserver left is less than DUST amount.     
        vm.expectRevert("Insufficient reserves after liquidity removal");
        collateralAndLiquidity.liquidateUser(alice);

    }

add this test inside collateralAndLiquidity.t.sol and run with command COVERAGE="yes" NETWORK="sep" forge test --mt testUserIsNotLiquidatable -vv --rpc-url https://rpc.sepolia.org/.

Tools Used

Manual Review

Recommended Mitigation Steps

To handle this edge case is totally design decision ,I can offer one recommendation here. add one more argument of type bool to removeLiquidity function. which will be used to identify when to check for DUST value. In case of liquidateUser pass its value false and in other cases pass its value true.

diff --git a/src/pools/Pools.sol b/src/pools/Pools.sol
index 7adb563..28168fa 100644
--- a/src/pools/Pools.sol
+++ b/src/pools/Pools.sol
@@ -167,7 +167,7 @@ contract Pools is IPools, ReentrancyGuard, PoolStats, ArbitrageSearch, Ownable
 
        // Remove liquidity for the user and reclaim the underlying tokens
        // Only callable from the CollateralAndLiquidity contract - so it can specify totalLiquidity with authority
-       function removeLiquidity( IERC20 tokenA, IERC20 tokenB, uint256 liquidityToRemove, uint256 minReclaimedA, uint256 minReclaimedB, uint256 totalLiquidity ) external nonReentrant returns (uint256 reclaimedA, uint256 reclaimedB)
+       function removeLiquidity( IERC20 tokenA, IERC20 tokenB, uint256 liquidityToRemove, uint256 minReclaimedA, uint256 minReclaimedB, uint256 totalLiquidity, bool checkReserves ) external nonReentrant returns (uint256 reclaimedA, uint256 reclaimedB)

Liquidity.sol

diff --git a/src/staking/Liquidity.sol b/src/staking/Liquidity.sol
@@ -125,7 +125,7 @@ abstract contract Liquidity is ILiquidity, StakingRewards
 
                // Remove the amount of liquidity specified by the user.
                // The liquidity in the pool is currently owned by this contract. (external call)
-               (reclaimedA, reclaimedB) = pools.removeLiquidity( tokenA, tokenB, liquidityToWithdraw, minReclaimedA, minReclaimedB, totalShares[poolID] );
+               (reclaimedA, reclaimedB) = pools.removeLiquidity( tokenA, tokenB, liquidityToWithdraw, minReclaimedA, minReclaimedB, totalShares[poolID], true );
 

CollateralAndLiquidity.sol

diff --git a/src/stable/CollateralAndLiquidity.sol b/src/stable/CollateralAndLiquidity.sol
@@ -148,7 +148,7 @@ contract CollateralAndLiquidity is Liquidity, ICollateralAndLiquidity

               // Withdraw the liquidated collateral from the liquidity pool.
               // The liquidity is owned by this contract so when it is withdrawn it will be reclaimed by this contract.
-               (uint256 reclaimedWBTC, uint256 reclaimedWETH) = pools.removeLiquidity(wbtc, weth, userCollateralAmount, 0, 0, totalShares[collateralPoolID] );
+               (uint256 reclaimedWBTC, uint256 reclaimedWETH) = pools.removeLiquidity(wbtc, weth, userCollateralAmount, 0, 0, totalShares[collateralPoolID] , false);

Assessed type

DoS

Lines of code

https://github.com/code-423n4/2024-01-salty/blob/53516c2cdfdfacb662cdea6417c52f23c94d5b5b/src/pools/Pools.sol#L222

Vulnerability details

Impact

The Pools.sol contract allow users to deposits and withdraw tokens before and after swapping. however the issue arises if the amount in Pool is less than DUST amount the user will not be able to withdraw it. imagine a case where swap return small value i.e amount<DUST of other token or user withdraw an amount which in result left userDeposits<DUST. The pool can eventually have a very large number of users with these DUST deposits. although The amount for one user is very tiny , but the impact increases with the number of users.

 // @audit-issue : user will not be able to withdraw if mistakenly dust amount remain left
    function withdraw( IERC20 token, uint256 amount ) external nonReentrant
    	{
    	require( _userDeposits[msg.sender][token] >= amount, "Insufficient balance to withdraw specified amount" );
@>       require( amount > PoolUtils.DUST, "Withdraw amount too small");

		_userDeposits[msg.sender][token] -= amount;

    	// Send the token to the user
    	token.safeTransfer( msg.sender, amount );

    	emit TokenWithdrawal(msg.sender, token, amount);
    	}

Proof of Concept

   function testFailDidNotAllowUserToWithDrawDustLeft() public {
	vm.startPrank(address(0x1234));
	 TestERC20 token = new TestERC20("TEST", 18);
	 uint256 initialBalance = token.balanceOf(address(0x1234));
	 // Test deposit
	 uint256 depositAmount = 1 ether;
	 token.approve(address(pools), 1000 ether);
	 pools.deposit(token, depositAmount);
	 assertEq(token.balanceOf(address(0x1234)), initialBalance - depositAmount);
	 assertEq(pools.depositedUserBalance(address(0x1234), token), depositAmount);
	 uint256 withdrawAmount = depositAmount - PoolUtils.DUST; 
	 pools.withdraw(token, withdrawAmount);
	 assertEq(token.balanceOf(address(0x1234)), initialBalance - depositAmount + withdrawAmount);
	 assertEq(pools.depositedUserBalance(address(0x1234), token), PoolUtils.DUST);
		// @audit: the user has left DUST value in Pool , but he is not able to withdraw it.
	 pools.withdraw(token, pools.depositedUserBalance(address(0x1234) , token));

 }

Add this test inside Pools.t.sol and run with command COVERAGE="yes" NETWORK="sep" forge test --mt testFailDidNotAllowUserToWithDrawDustLeft -vv --rpc-url https://rpc.sepolia.org/.

Tools Used

Manual Review

Recommended Mitigation Steps

Remove The DUST amount check form Pools.withdraw funciton.

diff --git a/src/pools/Pools.sol b/src/pools/Pools.sol
index 7adb563..07fc769 100644
--- a/src/pools/Pools.sol
+++ b/src/pools/Pools.sol
 function withdraw( IERC20 token, uint256 amount ) external nonReentrant
        {
        require( _userDeposits[msg.sender][token] >= amount, "Insufficient balance to withdraw specified amount" );
-        require( amount > PoolUtils.DUST, "Withdraw amount too small");

Assessed type

Other

Lines of code

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

Vulnerability details

Impact

The protocol imposes a Dust invariant on reserves in a pool, ensuring that the reserve will never fall below the DUST value. However, there is a missing check for reserves.reserve1 >= PoolUtils.DUST, causing the reserves in the pool related to reserves1 to consistently violate this invariant.

function removeLiquidity( IERC20 tokenA, IERC20 tokenB, uint256 liquidityToRemove, uint256 minReclaimedA, uint256 minReclaimedB, uint256 totalLiquidity ) external nonReentrant returns (uint256 reclaimedA, uint256 reclaimedB)
		{
		...
		// Make sure that removing liquidity doesn't drive either of the reserves below DUST.
		// This is to ensure that ratios remain relatively constant even after a maximum withdrawal.
		// @audit-issue :  reserves.reserve1 >= PoolUtils.DUST)
@>        require((reserves.reserve0 >= PoolUtils.DUST) && (reserves.reserve0 >= PoolUtils.DUST), "Insufficient reserves after liquidity removal");
    
    ...
		}

Proof of Concept

    function testNewMaximumLiquidityRemoval() public {
            vm.startPrank(address(collateralAndLiquidity));
            IERC20 tokenA = new TestERC20( "TEST", 18 );
            IERC20 tokenB = new TestERC20( "TEST", 18 );
            vm.stopPrank();
    
            vm.prank(address(dao));
            poolsConfig.whitelistPool( pools, tokenA, tokenB);
    
            vm.startPrank(address(collateralAndLiquidity));
            tokenA.approve(address(pools), type(uint256).max);
            tokenB.approve(address(pools), type(uint256).max);
    
            pools.addLiquidity(tokenA, tokenB, 200, 110 , 0, 0);
    
            // uint256 totalShares = 200 ether;
            pools.removeLiquidity(tokenA, tokenB, 100, 0, 0, 310);
    
            (uint256 reservesA, uint256 reservesB) = pools.getPoolReserves(tokenA, tokenB);
    
            // // Ensure that DUST remains
            assert( reservesA > PoolUtils.DUST );
            // @audit : Reserver falls below DUST amount

            assert( reservesB < PoolUtils.DUST );
    
            }

Add this testcase in Pools.t.sol and run with command : COVERAGE="yes" NETWORK="sep" forge test --mt testNewMaximumLiquidityRemoval -vv --rpc-url https://rpc.sepolia.org)

Tools Used

Recommended Mitigation Steps

I beleive that it is a typo. add check for reserve1 as below:

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

Assessed type

Invalid Validation