Salty.IO - djxploit's results

Lines of code

https://github.com/code-423n4/2024-01-salty/blob/main/src/staking/StakingRewards.sol#L67

Vulnerability details

Impact

The main impact of this vulnerability is that liquidators will not be able to liquidate the positions. And as time passes on, this bad debt will ultimately make the collaterals insolvent. This will affect the protocol, as it will lose funds.

In CollateralAndLiquidity contract, when a user deposits collateral through depositCollateralAndIncreaseShare , he can borrow USDS. Let's assume that the user has borrowed X amt of USDS which is maximum amount he can borrow against his deposited collateral. One thing to note here is that when depositCollateralAndIncreaseShare is called, it will trigger a cooldown period (currently 1 hour). This is a security mechanism to prevent reward hunting. Now the user can't increase or decrease his shares for 1 hour.

Now say the market has crashed, and the price of WETH and WBTC has gone down. Due to this, the user's position has also become liquidateble. So, a liquidator calls liquidateUser to liquidate the user's position. This will remove the liquidity and decrease the user's share.

But the user can prevent his position from getting liquidated, by frontrunning the liquidateUser function call and depositing 101 wei of WETH and WBTC using the depositCollateralAndIncreaseShare function first. Now as the depositCollateralAndIncreaseShare function will get executed first because of front-running, so this will trigger the cooldown period of 1 hour. Now after this transaction, the liquidateUser function will get executed, but it will revert because of the cooldown period. That is because, the liquidateUser function calls _decreaseUserShare function which will check if block.timestamp >= user.cooldownExpiration , and as this will be false, so the transaction will revert.

Now the user can continue preventing his position from liquidation, by front-running liquidators, or he can submit 101 wei after every stakingConfig.modificationCooldown() time, so that the cooldown period never expires.

In the meantime, the price of WETH and WBTC may go further down, which may make the position insolvent. This means that the protocol will surely lose funds.

Proof of Concept

A test case is presented below, representing how the functions would be executed when front-run. It is a representation , i.e I have not showcased how to do the front-run by analysing mempool , and have simply shown the order of execution of the functions when front-runned.

function testcounter() external {
  // charlie is a random user depositing collateral
  vm.startPrank( charlie );
  collateralAndLiquidity.depositCollateralAndIncreaseShare(wbtc.balanceOf(charlie), weth.balanceOf(charlie), 0, block.timestamp, false );
  vm.stopPrank();

  // alice is the attacker account
  vm.startPrank( alice );		 
  collateralAndLiquidity.depositCollateralAndIncreaseShare(wbtc.balanceOf(alice)/3, weth.balanceOf(alice)/3, 0, block.timestamp, false );
  uint256 maxUSDS = collateralAndLiquidity.maxBorrowableUSDS(alice);
  collateralAndLiquidity.borrowUSDS( maxUSDS);
  vm.stopPrank();

  // Market crashes
  _crashCollateralPrice();
  vm.warp( block.timestamp + 1 days );

  // Alice frontruns and deposits 101 wei before the liquidator tries to liquidate his position
   vm.startPrank(alice);
   bool x = collateralAndLiquidity.canUserBeLiquidated(alice);
   console.log(x);
   collateralAndLiquidity.depositCollateralAndIncreaseShare(101, 101, 0, block.timestamp, false );
   vm.stopPrank();
     
   // this will revert because of the cooldown revert   
   vm.startPrank(charlie);
   collateralAndLiquidity.liquidateUser(alice);
   vm.stopPrank();
 }

Tools Used

Manula review

Recommended Mitigation Steps

Cooldown period should not affect liquidation process. Make exception.

Assessed type

Timing

Lines of code

https://github.com/code-423n4/2024-01-salty/blob/main/src/stable/CollateralAndLiquidity.sol#L128

Vulnerability details

Impact

In CollateralAndLiquidity contract, the function repayUSDS and liquidateUser can be DOSed by a motivated attacker, by repeatedly calling borrowUSDS and then repayUSDS functions.

Consider a motivated attacker has deposited WBTC/WETH liquidity, and this allowed him to borrow X amount of USDS. So he call borrowUSDS with X amt which gives him X USDS tokens. He then repays the X amt of USDS by calling repayUSDS. This increases the usdsThatShouldBeBurned variable (in liquidizer contract) by X amt, due to #L128 in repayUSDS. A motivated attacker can now call borrowUSDS and repayUSDS, until the value of usdsThatShouldBeBurned is about to cross type(uint256).max.

So now when a legit user will try to call repayUSDS , it will try to increment usdsThatShouldBeBurned variable , which will revert because of overflow. Hence the repay functionality would be DOSed.

Similarly liquidateUser function will also be DOSed due to #L181

Proof of Concept

Given below a test case where it is shown how to call borrowUSDS and repayUSDS. The below test case only calls these functions 1000 times, but to cause the overflow , a motivated attacker needs to call the below test case multiple times in batches.

function testcounter() external {
    vm.startPrank( alice );		 
    collateralAndLiquidity.depositCollateralAndIncreaseShare(wbtc.balanceOf(alice), weth.balanceOf(alice), 0, block.timestamp, true );
    uint count1 = liquidizer.usdsThatShouldBeBurned();
    console.log(count1);
    for(uint i = 0;i<1000;i++) {
	    uint256 maxUSDS = collateralAndLiquidity.maxBorrowableUSDS(alice);
		    collateralAndLiquidity.borrowUSDS( maxUSDS );
            collateralAndLiquidity.repayUSDS(maxUSDS);
        }
    uint count = liquidizer.usdsThatShouldBeBurned();
    console.log(count);
    vm.stopPrank();
    }

Tools Used

Manual review

Recommended Mitigation Steps

In line : https://github.com/code-423n4/2024-01-salty/blob/main/src/stable/CollateralAndLiquidity.sol#L125

send the usds tokens to address(liquidizer) instead of address(usds)

Assessed type

DoS

Lines of code

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

Vulnerability details

Impact

A user's position will not be liquidated for the 1st depositor, because of the revert statement in #L187 of Pools contract.

Consider that the protocol has been deployed recently. UserA being its 1st user to deposit collateral to pool collateralPoolID. Now userA borrows USDS. Now consider that price of WETH/WBTC crashes, and user's position is liquitable. A liquidator when tries to liquidate the position, he will not be able to do so, because of the revert statement in #L187 of Pools contract. As the reserves for token A and token B for pool with ID collateralPoolID is equal to the collateral deposited by the depositor, hence removing the collaterals would trigger the revert statement.

This will prevent liquidator to liquidate the position of 1st depositor, untill no other collaterals are deposited.

Proof of Concept

function testliquidation() external {
        vm.startPrank( alice );
		collateralAndLiquidity.depositCollateralAndIncreaseShare(wbtc.balanceOf(alice), weth.balanceOf(alice), 0, block.timestamp, true );
        
	uint256 maxUSDS = collateralAndLiquidity.maxBorrowableUSDS(alice);
	collateralAndLiquidity.borrowUSDS( maxUSDS );
        _crashCollateralPrice();
        vm.warp( block.timestamp + 1 days );
        vm.prank(bob);
        collateralAndLiquidity.liquidateUser(alice);
	vm.stopPrank();
    }

Tools Used

Manual review

Recommended Mitigation Steps

Handle the edge case for the 1st collateral depositor

Assessed type

Error

Lines of code

https://github.com/code-423n4/2024-01-salty/blob/main/src/stable/CollateralAndLiquidity.sol#L151

Vulnerability details

Impact

An attacker can grief a liquidator by front-running liquidateUser function. This exposes the liquidator to unnecessary slippage.

In function liquidateUser, in #L151, while removing liquidity of the liquidated user, the minimum reclaimable tokenA and tokenB are set to 0. This ultimately means that the liquidator is open to 100% slippage tolerance. This allows an attacker to front-run liquidateUser function , resulting in lesser reclaimed token A and B , which in-turn gives less rewards to the liquidator.

Thus the missing slippage results in loss for liquidator(receives less reward) and also for protocol(reclaims less tokenA and tokenB)

Proof of Concept

Consider the situation where :

1. Liquidator calls liquidateUser to liquidate a user's position
2. Attacker see this and delay the execution of the above transaction (by frontrunning), such that the transaction gets executed when the value of reclaimedA or reclaimedB would be lowest.
3. This results in liquidator getting less reward than originally intended, due to the unwanted slippage
4. Also the protocol has reclaimed less tokenA and tokenB (for burning) than originally intended due to the slippage

Tools Used

Manual review

Recommended Mitigation Steps

Take the minimum reclaimable tokenA and tokenB parameters as arguments, and ask the liquidator to fill it.

Assessed type

MEV

Lines of code

https://github.com/code-423n4/2024-01-salty/blob/main/src/stable/CollateralAndLiquidity.sol#L172

Vulnerability details

Impact

WBTC has pause functions, which allows it to be paused. This will prevent transferring of WBTC, which will affect the liquidation process and liquidity withdrawal process. Furthermore, as salt.io doesn't have any pause functionality, so even if WBTC is paused , the protocol would be live.
This would affect user's who would want to withdraw WBTC collateral and for protocol, this would prevent the protocol from liquidating any position because of the transfer statement in #L172

Proof of Concept

Consider the below situation where the liquidator will not be able to liquidate a position: User deposited WETH/WBTC collateral User borrowed X USDS against this collateral Market crashed, and user's position got below liquidation ratio Liquidator sees this and tried to liquidate the position, but got reverted because of the wbtc transfer statement in #L172, that will fail (as wbtc is paused)

Hence liquidator will not be able to liquidate positions

Tools Used

Manual review

Recommended Mitigation Steps

Add a functionality to pause the protocol by the DAO

Assessed type

DoS