Salty.IO - Kaysoft's results

Lines of code

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

Vulnerability details

Impact

Loss of fund to the protocol and dilution of the USDS stable coin value

Scenario

1. When any user's position is liquiditable
2. Alice calls the liquidateUser(...) function to liquidate the user
3. The removeliquidity function call in the liquidateUser(...) function has zero passed as minimumAmountOut for both Weth and Wbtc tokens so an attacker can send a transaction to manipulate to pool before Alice's transaction then another then another transaction after Alice's transaction.
4. This would allow the attacker profit from Alice's transaction due to lack of slippage with sandwich attack.

Proof of Concept

The liquidateUser(...) function calls the removeLiquidity(...) function but passed zero for both WETH and WBTC minimum acceptible token. This is an opportunity for attackers to setup flashbots to manipulate the pools before every liquidation profiting from it can causing losses.

This would also dilute/depeg the value of USDS in circulation because lesser than the defaulting position's collateral value may be returned from the remove liquidity operation which would send 5% of it to the liquidator before burning the remaining. The burned amount maybe lesser than the collateral value devaluing the USDC in circulation.

File: CollateralAndLiqudity.sol
(uint256 reclaimedWBTC, uint256 reclaimedWETH) = pools.removeLiquidity(wbtc, weth, userCollateralAmount, 0, 0, 

File: stable/CollateralAndLiqudity.sol
function liquidateUser( address wallet ) external nonReentrant
		{
		require( wallet != msg.sender, "Cannot liquidate self" );

		// First, make sure that the user's collateral ratio is below the required level
		require( canUserBeLiquidated(wallet), "User cannot be liquidated" );

		uint256 userCollateralAmount = userShareForPool( wallet, collateralPoolID );

		// 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] );

		// Decrease the user's share of collateral as it has been liquidated and they no longer have it.
		_decreaseUserShare( wallet, collateralPoolID, userCollateralAmount, true );

		// The caller receives a default 5% of the value of the liquidated collateral.
		uint256 rewardPercent = stableConfig.rewardPercentForCallingLiquidation();

		uint256 rewardedWBTC = (reclaimedWBTC * rewardPercent) / 100;
		uint256 rewardedWETH = (reclaimedWETH * rewardPercent) / 100;

		// Make sure the value of the rewardAmount is not excessive
		uint256 rewardValue = underlyingTokenValueInUSD( rewardedWBTC, rewardedWETH ); // in 18 decimals
		uint256 maxRewardValue = stableConfig.maxRewardValueForCallingLiquidation(); // 18 decimals
		if ( rewardValue > maxRewardValue )
			{
			rewardedWBTC = (rewardedWBTC * maxRewardValue) / rewardValue;
			rewardedWETH = (rewardedWETH * maxRewardValue) / rewardValue;
			}

		// Reward the caller
		wbtc.safeTransfer( msg.sender, rewardedWBTC );
		weth.safeTransfer( msg.sender, rewardedWETH );

		// Send the remaining WBTC and WETH to the Liquidizer contract so that the tokens can be converted to USDS and burned (on Liquidizer.performUpkeep)
		wbtc.safeTransfer( address(liquidizer), reclaimedWBTC - rewardedWBTC );
		weth.safeTransfer( address(liquidizer), reclaimedWETH - rewardedWETH );

		// Have the Liquidizer contract remember the amount of USDS that will need to be burned.
		uint256 originallyBorrowedUSDS = usdsBorrowedByUsers[wallet];
		liquidizer.incrementBurnableUSDS(originallyBorrowedUSDS);

		// Clear the borrowedUSDS for the user who was liquidated so that they can simply keep the USDS they previously borrowed.
		usdsBorrowedByUsers[wallet] = 0;
		_walletsWithBorrowedUSDS.remove(wallet);

		emit Liquidation(msg.sender, wallet, reclaimedWBTC, reclaimedWETH, originallyBorrowedUSDS);
		}

The require statement on line 193 is used to protect remove liquidity transactions from slippage loss this way

File: src/pools/Pools.sol
function removeLiquidity(...){
...
193: require( (reclaimedA >= minReclaimedA) && (reclaimedB >= minReclaimedB), "Insufficient underlying tokens returned" );
...
}

Tools Used

Foundry

Recommended Mitigation Steps

Consider allowing the minimumAmountOut of both tokens be passed as parameter to the liquidateUser(...) parameter so that the frontend interface can provide a liquidator with current prices and slippage adjustment just like the frontend of DEXes.

Assessed type

MEV

[L-1] Lack of deadline check on add and removing liquidity.

There are 2 instances of this

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

The ensureNotExpired modifier is used on the Pool.swap() function but not used on the addLiquidity(...) and removeLiquidity(...) functions of Pool.sol

The ensureNotExpired modifier is used to protect transactions from being kept by the miners till it is profitable for them before execution especially in unstable market swings.

File: Pool.sol
modifier ensureNotExpired(uint deadline)
		{
		require(block.timestamp <= deadline, "TX EXPIRED");
		_;
		}

File: Pool.sol
function addLiquidity( ... ) external nonReentrant returns (uint256 addedAmountA, uint256 addedAmountB, uint256 addedLiquidity){
...
}

However, the ensureNotExpired modifier is not added to the addLiquidity(...) and removeLiquidity(...) functions.

Impact: removeLiquidity and addLiquidity transactions can be "saved for later" for as long as possible until it incurs enough loss based on slippage.

Recommendation: add a deadline parameter and the ensureNotExpired modifier to the addLiquidity(...) and removeLiquidity(...) functions as it is done on the swap(...) function.

function addLiquidity( ..., deadline) external nonReentrant ensureNotExpired(deadline) returns (uint256 addedAmountA, uint256 addedAmountB, uint256 addedLiquidity){
...
}

[L-2] Use the available private visibility specifier instead of creating the onlySameContract modifier

There are 11 instances of this

• https://github.com/code-423n4/2024-01-salty/blob/53516c2cdfdfacb662cdea6417c52f23c94d5b5b/src/Upkeep.sol#L95C2-L112C58
• https://github.com/code-423n4/2024-01-salty/blob/53516c2cdfdfacb662cdea6417c52f23c94d5b5b/src/Upkeep.sol#L145C2-L231C43

According the to Solidity docs:

private function: only visible in the current contract

And here is the onlySameContract modifier:

File:Upkeep.sol
modifier onlySameContract()
		{
    	require(msg.sender == address(this), "Only callable from within the same contract");
    	_;
		}

It is better to user the private keyword instead of creating and using a new modifier that achieves the same aim as the private visibility specifier.

Impact: More unnecessary code when private can be used.

Recommedation: Consider using private visibility specifier on functions step1(...) to step11(...).

--     function step1() public onlySameContract
++     function step1() private
		{
		collateralAndLiquidity.liquidizer().performUpkeep();
		}

[L-3] The Ownable library is unnecessary on the USDS.sol contract.

There are 2 instance of this

• https://github.com/code-423n4/2024-01-salty/blob/53516c2cdfdfacb662cdea6417c52f23c94d5b5b/src/stable/USDS.sol#L14
• https://github.com/code-423n4/2024-01-salty/blob/53516c2cdfdfacb662cdea6417c52f23c94d5b5b/src/stable/Liquidizer.sol#L21

The onlyOwner modifier of the ownable contract was used only once in the function that is to be executed only once at deployment, then ownership is renounced in the same function.

Since the below function is the only function where onlyOwner modifier is used and it is executed only once at deployment according to the comment above it, the code in the setCollateralAndLiquidity(...) can be implemented in the constructor since the constructor is run only once and can only be by the deployer.

File: src/stable/USDS.sol
28: // This will be called only once - at deployment time
29:	function setCollateralAndLiquidity( ICollateralAndLiquidity _collateralAndLiquidity ) external onlyOwner
30:		{
31:		collateralAndLiquidity = _collateralAndLiquidity; //@audit set this in the constructor since it will be called at deployment.

32:		// setCollateralAndLiquidity can only be called once
33:		renounceOwnership();//@audit ownable is unnecessary.
34:		}

The same applies to the Liquizer.sol contract and the Liquidizer.sol#setContracts(...) function.

• https://github.com/code-423n4/2024-01-salty/blob/53516c2cdfdfacb662cdea6417c52f23c94d5b5b/src/stable/Liquidizer.sol#L21

Impact: Adds more unnecessary codes that comes with the Ownable contract to the USDC.sol contract increasing complexity and deployment gas cost.

Recommendation: Implement the code in the setCollateralAndLiquidity(...) in the constructor of the contract this way:

constructor(ICollateralAndLiquidity _collateralAndLiquidity) ERC20( "USDS", "USDS" ) {
   collateralAndLiquidity = _collateralAndLiquidity;// also emit event.
}

The above will remove the need for the setCollateralAndLiquidity(...) and the Ownable contract codes.

[L-4] Using block.timestamp as deadline does not protect the transaction.

There are 2 instances of this

• https://github.com/code-423n4/2024-01-salty/blob/53516c2cdfdfacb662cdea6417c52f23c94d5b5b/src/staking/Liquidity.sol#L62
• https://github.com/code-423n4/2024-01-salty/blob/53516c2cdfdfacb662cdea6417c52f23c94d5b5b/src/pools/PoolUtils.sol#L67
• https://github.com/code-423n4/2024-01-salty/blob/53516c2cdfdfacb662cdea6417c52f23c94d5b5b/src/dao/DAO.sol#L321
• https://github.com/code-423n4/2024-01-salty/blob/53516c2cdfdfacb662cdea6417c52f23c94d5b5b/src/dao/DAO.sol#L372

The deadline is the Unix timestamp after which the transaction should revert to prevent transactions to be executed at a later timestamp than the specified.

However block.timestamp is dynamic because even after 5 hours block.timestamp will always the timestamp at the time the transaction was executed and the below check in the ensureNotExpired would pass at any timestamp.

File: Pools.sol
modifier ensureNotExpired(uint deadline)
		{
		require(block.timestamp <= deadline, "TX EXPIRED");
		_;
		}

To solve this situation the timestamp should be passed as input offchain. Frontends do this with javascript with Math.floor(Date.now() / 1000) and add 20minutes to it.

Impact: Miners can keep transactions till it incurs loss for the user for longer period because block.timestamp is dynamic.

Recommendation: The deadline should passed as parameter and it will be something like: 1706638171.

[GAS-1] The Ownable library is unnecessary on the USDS.sol contract.

There is 1 instance of this

• https://github.com/code-423n4/2024-01-salty/blob/53516c2cdfdfacb662cdea6417c52f23c94d5b5b/src/stable/USDS.sol#L14

The onlyOwner modifier of the ownable contract was used only once in the function that is to be executed only once at deployment, then ownership is renounced in the same function.

Since the below function is the only function where onlyOwner modifier is used and it is executed only once at deployment according to the comment above it, the code in the setCollateralAndLiquidity(...) can be implemented in the constructor since the constructor is run only once and can only be by the deployer.

File: src/stable/USDS.sol
28: // This will be called only once - at deployment time
29:	function setCollateralAndLiquidity( ICollateralAndLiquidity _collateralAndLiquidity ) external onlyOwner
30:		{
31:		collateralAndLiquidity = _collateralAndLiquidity; //@audit set this in the constructor since it will be called at deployment.

32:		// setCollateralAndLiquidity can only be called once
33:		renounceOwnership();//@audit ownable is unnecessary.
34:		}

Impact: Reduces deployment gas cost because the Ownable contract code is removed.

Recommendation: Implement the code in the setCollateralAndLiquidity(...) in the constructor of the contract this way:

constructor(ICollateralAndLiquidity _collateralAndLiquidity) ERC20( "USDS", "USDS" ) {
   collateralAndLiquidity = _collateralAndLiquidity;// also emit event.
}

The above will remove the need for the setCollateralAndLiquidity(...) and the Ownable contract codes.