Salty.IO - Udsen's results

Lines of code

https://github.com/code-423n4/2024-01-salty/blob/main/src/stable/CollateralAndLiquidity.sol#L154 https://github.com/code-423n4/2024-01-salty/blob/main/src/stable/CollateralAndLiquidity.sol#L70-L76 https://github.com/code-423n4/2024-01-salty/blob/main/src/staking/Liquidity.sol#L107 https://github.com/code-423n4/2024-01-salty/blob/main/src/pools/Pools.sol#L146-L147 https://github.com/code-423n4/2024-01-salty/blob/main/src/staking/StakingRewards.sol#L64-L71 https://github.com/code-423n4/2024-01-salty/blob/main/src/staking/StakingRewards.sol#L104-L111

Vulnerability details

Impact

The CollateralAndLiquidity.liquidateUser function is used to liquidate a position which has fallen under the minimum collateral ratio. During the liquidateUser function execution the StakingRewards._decreaseUserShare function is called to decrease the user's share of collateral as it has been liquidated and they no longer have it, as shown below:

	_decreaseUserShare( wallet, collateralPoolID, userCollateralAmount, true );

As it is evident from the above code snippet the useCooldown bool is set to true. Hence the StakingRewards._decreaseUserShare function will perform the following check to ensure that the cool down period has expired.

	if ( useCooldown ) //@audit-info - check if the cooldown is mentioned
	if ( msg.sender != address(exchangeConfig.dao()) ) // DAO doesn't use the cooldown
		{
		require( block.timestamp >= user.cooldownExpiration, "Must wait for the cooldown to expire" );
		//@audit-info - ensure hte cool down period has passed
		// Update the cooldown expiration for future transactions
		user.cooldownExpiration = block.timestamp + stakingConfig.modificationCooldown();
		}

if the block.timestamp < user.cooldownExpiration the transaction will revert and the liquidation won't be executed.

A malicious borrower can exploit the user.cooldownExpiration to revert the liquidation attempts and thus keep his position alive until he can get his collateralization ratio above the minimumCollateralRatioPercent or can wait till his collateralization ratio is below < 100 thus incurring loss on the protocol. The malicious borrower can manipulate the user.cooldownExpiration to prevent the liquidation as explained below:

1. A malicious borrower calls the CollateralAndLiquidity.depositCollateralAndIncreaseShare and deposits collateral (WBTC/WETH) worth of USD 10,000 and borrows USDS 5000. (consider the initialCollateralRatioPercent = 200).
2. This will update the user.cooldownExpiration since the StakingRewards._increaseUserShare is called. The user.cooldownExpiration = block.timestamp + stakingConfig.modificationCooldown() is executed and the user.cooldownExpiration is set to one hour from the transaction execution time (block.timestamp).
3. The malicious borrower does not attempt to deposit or withdraw any collateral after this initial transaction and hence after one hour his user.cooldownExpiration will be expired.
4. After some time the deposited collateral value of the malicious borrower, in USD, drops significantly to USD 5450. This makes the collateralization ratio = (5450 / 5000) * 100 = 109 and it is < minimumCollateralRatioPercent (Assume minimumCollateralRatioPercent is the default value of 110).
5. A liquidator sees the opportunity and attempts to liquidate the malicious borrower.
6. Malicious borrower can easily front-run the liquidation transaction by calling the CollateralAndLiquidity.depositCollateralAndIncreaseShare function by depositing WBTC and WETH just more than PoolUtils.DUST (100) value. For example 101 tokens considering 18 decimal places.
7. This will call the StakingRewards._increaseUserShare and the user.cooldownExpiration will be incremented by another 1 hour.
8. As a result when the liquidation transaction is being executed the block.timestamp < user.cooldownExpiration will occur since the user.cooldownExpiration has not expired and the liquidation transaction will revert.
9. The malicious borrower can keep on reverting the liquidation transaction by front-running the liquidation and updating the user.cooldownExpiration by depositing WBTC and WETH tokens just more than the PoolUtils.DUST amount of 100 (Eg : 101).

The following impacts are possible as a result of the above attack vector:

1. This will put the protocol in danger since if the existing collateral value of the borrower falls steeply and collateralization ration goes below 100 then even if the liquidation is successfully executed the protocol will not able to fully recover the lent amount and thus results in loss of funds to the protocol.
2. Furthermore the malicious borrower can delay the liquidation as much as he wants and then can later gets his liquidation ratio above the minimumCollateralRatioPercent such that his position is secured thus getting unfair advantage.
3. The liquidator who is trying to liquidate this undercollateralized position will get his transaction reverted thus incurring transaction gas loss thus resulting in loss of funds to him rather than getting his due liquidation reward.

Proof of Concept

		_decreaseUserShare( wallet, collateralPoolID, userCollateralAmount, true );

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

	function depositCollateralAndIncreaseShare( uint256 maxAmountWBTC, uint256 maxAmountWETH, uint256 minLiquidityReceived, uint256 deadline, bool useZapping ) external nonReentrant ensureNotExpired(deadline)  returns (uint256 addedAmountWBTC, uint256 addedAmountWETH, uint256 addedLiquidity)
		{
		// Have the user deposit the specified WBTC/WETH liquidity and increase their collateral share
		(addedAmountWBTC, addedAmountWETH, addedLiquidity) = _depositLiquidityAndIncreaseShare( wbtc, weth, maxAmountWBTC, maxAmountWETH, minLiquidityReceived, useZapping );

		emit CollateralDeposited(msg.sender, addedAmountWBTC, addedAmountWETH, addedLiquidity);
		}

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

		_increaseUserShare( msg.sender, poolID, addedLiquidity, true );

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

		require( maxAmountA > PoolUtils.DUST, "The amount of tokenA to add is too small" );
		require( maxAmountB > PoolUtils.DUST, "The amount of tokenB to add is too small" );

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

		if ( useCooldown )
		if ( msg.sender != address(exchangeConfig.dao()) ) // DAO doesn't use the cooldown
			{
			require( block.timestamp >= user.cooldownExpiration, "Must wait for the cooldown to expire" );

			// Update the cooldown expiration for future transactions
			user.cooldownExpiration = block.timestamp + stakingConfig.modificationCooldown();
			}

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

		if ( useCooldown )
		if ( msg.sender != address(exchangeConfig.dao()) ) // DAO doesn't use the cooldown
			{
			require( block.timestamp >= user.cooldownExpiration, "Must wait for the cooldown to expire" );

			// Update the cooldown expiration for future transactions
			user.cooldownExpiration = block.timestamp + stakingConfig.modificationCooldown();
			}

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

Tools Used

Manual Review and VSCode

Recommended Mitigation Steps

The useCooldown bool value is used to prevent reward hunting (ie - quickly depositing and withdrawing large amounts of liquidity to snipe rewards as they arrive). And there is no reward hunting expected to be performed during the liquidaiton. Hence there is no advantage to be gained by setting the useCooldown bool value to true during the liquidation when calling the CollateralAndLiquidity.liquidateUser. Since the liquidation can be called by anyone other than the wallet which is being liquidated itself. Hence there is no undue advantage a user can gain via reward hunting during the liquidation. Hence we can set the useCooldown bool value to false when calling the StakingRewards._decreaseUserShare function inside the CollateralAndLiquidity.liquidateUser.

Hence the updated _decreaseUserShare function call is as shown below:

	_decreaseUserShare( wallet, collateralPoolID, userCollateralAmount, false );

This will ensure no cool down period check is performed during the liquidation transaction thus not letting a malicious user to revert the liquidation transaction by updating the user.cooldownExpiration timestamp by depositing WBTC and WETH just more than the DUST amount.

Assessed type

Other

Lines of code

https://github.com/code-423n4/2024-01-salty/blob/main/src/staking/StakingRewards.sol#L113-L118 https://github.com/code-423n4/2024-01-salty/blob/main/src/staking/StakingRewards.sol#L132-L133 https://github.com/code-423n4/2024-01-salty/blob/main/src/staking/StakingRewards.sol#L99

Vulnerability details

Impact

The StakingRewards._decreaseUserShare function is used to decrease a user's share for the pool and have any pending rewards sent to them. When the amount of pending rewards are calculated, initially the virtualRewardsToRemove are calculated as follows:

	uint256 virtualRewardsToRemove = (user.virtualRewards * decreaseShareAmount) / user.userShare;

Then the virtualRewardsToRemove is substracted from the rewardsForAmount value to calculate the claimableRewards amount as shown below:

	if ( virtualRewardsToRemove < rewardsForAmount )
		claimableRewards = rewardsForAmount - virtualRewardsToRemove; 

But the issue here is that the virtualRewardsToRemove calculation is rounded down in favor of the user and not in the favor of the protocol. Since the virtualRewardsToRemove is rounded down there is an opportunity to the user to call the StakingRewards._decreaseUserShare function with a very small decreaseShareAmount value such that the virtualRewardsToRemove will be rounded down to 0. Providing a very small decreaseShareAmount value is possible since only input validation on decreaseShareAmount is ! = 0 as shown below:

	require( decreaseShareAmount != 0, "Cannot decrease zero share" );

When the claimableRewards is calculated it will be equal to the rewardsForAmount value since the virtualRewardsToRemove will be 0. This way the user can keep on removing his liquidity from a particular pool by withdrawing small decreaseShareAmount at a time such that keeping virtualRewardsToRemove at 0 due to rounding down.

Furthermore the decreaseShareAmount value should be selected in such a way rewardsForAmount is calculated to a considerable amount after round down (not zero) and the virtualRewardsToRemove should round down to zero.

Hence as a result the user can withdraw all the rewardsForAmount as the claimableRewards even though some of those rewards are virtual rewards which should not be claimable as clearly stated by the following natspec comment:

	// Some of the rewardsForAmount are actually virtualRewards and can't be claimed.

Hence as a result the user is able to get an undue advantage and claim more rewards for his liquidity during liquidity withdrawable. This happens because the user can bypass the virtual reward subtraction by making it round down to 0. As a result the virtualReward amount of the rewardsForAmount, which should not be claimable is also claimed by the user unfairly.

Proof of Concept

		// Determine the share of the rewards for the amountToDecrease (will include previously added virtual rewards)
		uint256 rewardsForAmount = ( totalRewards[poolID] * decreaseShareAmount ) / totalShares[poolID];

		// For the amountToDecrease determine the proportion of virtualRewards (proportional to all virtualRewards for the user)
		// Round virtualRewards down in favor of the protocol
		uint256 virtualRewardsToRemove = (user.virtualRewards * decreaseShareAmount) / user.userShare;

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

		if ( virtualRewardsToRemove < rewardsForAmount )
			claimableRewards = rewardsForAmount - virtualRewardsToRemove;

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

		require( decreaseShareAmount != 0, "Cannot decrease zero share" );

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

Tools Used

Manual Review and VSCode

Recommended Mitigation Steps

Hence it is recommended to round up the virtualRewardsToRemove value during its calculation such that it will not be rounded down to zero for a very small decreaseShareAmount. This way user is unable to claim the rewards which he is not eligible for and the rewards will be claimed after accounting for the virtual rewards.

Assessed type

Other

Lines of code

https://github.com/code-423n4/2024-01-salty/blob/main/src/price_feed/PriceAggregator.sol#L37-L44 https://github.com/code-423n4/2024-01-salty/blob/main/src/price_feed/PriceAggregator.sol#L50-L51 https://github.com/code-423n4/2024-01-salty/blob/main/src/price_feed/PriceAggregator.sol#L122-L123 https://github.com/code-423n4/2024-01-salty/blob/main/src/price_feed/PriceAggregator.sol#L151-L158

Vulnerability details

Impact

The PriceAggregator.setInitialFeeds function is used to set the initial price feeds and only be called by the owner. And this function can only be called once by the owner as it is depicted in the following conditional check.

	require( address(priceFeed1) == address(0), "setInitialFeeds() can only be called once" );

And if the owner needs to change the priceFeeds there after he will have to call the PriceAggregator.setPriceFeed function which has a price modification cool down period implemented. This is why the setInitialFeeds is implemented as a function that can be called only once.

And the way the priceAggregator._aggregatePrices function works, even two price feeds will be enough for the price aggregator to work properly since the following condition will be bypassed with two valid price feeds.

	if ( numNonZero < 2 )
		return 0;

Hence the owner can call PriceAggregator.setInitialFeeds function multiple times to change the priceFeed2 and priceFeed3 as he needs without complementing the price modification cool down period.

Let's consider the following scenario:

1. The owner calls the PriceAggregator.setInitialFeeds function with priceFeeds1 == address(0) and provides valid price feed addresses to _priceFeed2 and _priceFeed3.
2. As a result the priceFeed1 will remain == address(0).
3. Owner can now bypass the require( address(priceFeed1) == address(0), "setInitialFeeds() can only be called once" ) check to call the PriceAggregator.setInitialFeeds function to change the priceFeed2 and priceFeed3 as he wishes without complementing the price modification cool down period
4. Even with the priceFeed1 being address(0) the price aggregator will work fine since call to the priceFeed1 (in the PriceAggregator._getPriceBTC and priceAggregator._getPriceETH functions) is performed in a try-catch block and as a result the call to address(0) will not revert the transaction.
5. And further two priceFeeds are enough for the priceAggregator contract to work properly.

Hence if the owner requires he can decide to keep on changing the priceFeed2 and priceFeed3 as he needs with out complementing the price modification cool down period. This will make the PriceAggregator.setPriceFeed function less useful since its price modification cool down period implementation can be bypassed by the owner easily, to change the priceFeeds as the owner wishes. And furthermore this should not be considered as a admin mistake since the PriceAggregator.setInitialFeeds function is designed in such a way that even the owner is not allowed to call the function more than once. This vulnerability clearly shows how the owner can call the PriceAggregator.setInitialFeeds function to change the priceFeeds2 and priceFeeds3 multiple times contrary to what the function is designed to do.

Note : Even though the L1 or the automated report states User facing functions should have address(0) checks it does not mention this exact vulnerability and the impact of it.

Proof of Concept

	function setInitialFeeds( IPriceFeed _priceFeed1, IPriceFeed _priceFeed2, IPriceFeed _priceFeed3 ) public onlyOwner
		{
		require( address(priceFeed1) == address(0), "setInitialFeeds() can only be called once" );

		priceFeed1 = _priceFeed1;
		priceFeed2 = _priceFeed2;
		priceFeed3 = _priceFeed3;
		}

https://github.com/code-423n4/2024-01-salty/blob/main/src/price_feed/PriceAggregator.sol#L37-L44

		if ( block.timestamp < priceFeedModificationCooldownExpiration )
			return;

https://github.com/code-423n4/2024-01-salty/blob/main/src/price_feed/PriceAggregator.sol#L50-L51

		if ( numNonZero < 2 )
			return 0;

https://github.com/code-423n4/2024-01-salty/blob/main/src/price_feed/PriceAggregator.sol#L122-L123

 		try priceFeed.getPriceBTC() returns (uint256 _price)
			{
			price = _price;
			}
		catch (bytes memory)
			{
			// price remains 0
			}

https://github.com/code-423n4/2024-01-salty/blob/main/src/price_feed/PriceAggregator.sol#L151-L158

Tools Used

Manual Review and VSCode

Recommended Mitigation Steps

Hence it is recommended to implement an address(0) check for the _priceFeed1 address in the PriceAggregator.setInitialFeeds function such that the transaction will revert if the address(0) is passed in as the _priceFeed1. This will ensure that the _priceFeed1 is set to a non-zero value and the PriceAggregator.setInitialFeeds can only be called once as a result.

require(address(_priceFeed1) != address(0), "_priceFeed1 can not be address(0)");

Assessed type

Invalid Validation