Salty.IO - Banditx0x's results

Lines of code

https://github.com/code-423n4/2024-01-salty/blob/53516c2cdfdfacb662cdea6417c52f23c94d5b5b/src/price_feed/PriceAggregator.sol#L108-L146

Vulnerability details

Impact

Liquidations will be won by liquidators who manipulate the Salty reserve oracle to unfairly liquidate "almost-unhealthy" borrowers.

Proof of Concept

Liquidation is a race, where there is strong incentive for liquidators to be the first to liquidate a position. This means that if an oracle used for liquidation is even slightly manipulatable in a block, then this can be maliciously used to push a position which is near liquidation but not-yet-unhealthy into liquidation range.

The aggregated Oracle uses 3 price sources - Chainlink, Uniswap TWAP and Salty Reserves. It then takes the average of the two oracle prices that are closer together.

Chainlink and Uniswap TWAP Reserves are impractically expensive/risky to manipulate for this scenario. However, there price sources often return values which are +/- 0-5% of each other as the Uniswap TWAP is slightly time-lagged. It incorperates data up to 30 minutes from the past.

The salty reserves are very easy to manipulate, especially for the small % required to unfairly to manipulate almost-unhealty users.

For example:

Uniswap TWAP returns price (exchange rate) of $102 Chainlink oracle returns price of $100 Salty Reserves Oracle also returns price of $100

The current aggregated oracle returns average of 2 closest values which are both 100, so the result is 100.

Attacker manipulates Salty Reserves Oracle to $103. Now, the two closest prices are $102 and $103 so the aggregated oracle now returns $102.5

Note that this scenario happens ALL THE TIME. Let's say the value of a LP position is gradually decreasing. The TWAP oracle will lag behind the chainlink price oracle as it has the time delay. Therefore the liquidation can always be won by a liquidator who manipulates the salty reserves.

Tools Used

Manual Review

Recommended Mitigation Steps

Using 3 oracles is still recommended as if 1 oracle returns an outlier value, it can be ignored by averaging the other 2 oracles. However, an oracle other than the salty reserves should be used.

Assessed type

Oracle

Lines of code

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

Vulnerability details

Impact

Liquidations can be prevented by a user increasing or decreasing their share amount to reset the cooldown for decreasing shares.

Proof of Concept

In the liquidateUser function, the call to _decreaseUserShare passes in true for the useCooldown parameter. This means that any calls to liquidate will revert when the user's position is in cooldown.

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" );


			user.cooldownExpiration = block.timestamp + stakingConfig.modificationCooldown();
			}

A user can easily trigger the cooldownExpiration by calling depositCollateralAndIncreaseShare. A user can deposit liquidity even if their position is currently liquidatable. The cooldownExpiration for increasing and decreasing shares is the same, so this affects liquidations at all. Therefore the user can prevent liquidations on their insolvent positions indefinitely.

Tools Used

Manual review

Recommended Mitigation Steps

The fix is simple: the bool value false rather than true should be used when calling the _decreaseUserShare() function within liquidateUser

Assessed type

DoS

Lines of code

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

Vulnerability details

Impact

Users get slightly more rewards than they are entitled to, especially if they make multiple withdrawals. This can cause the last withdrawal to revert as there are not enough rewards to pay them out, causing the subtraction of rewards to underflow.

Proof of Concept

Rewards are tracked from the beginning of time, and then virtual rewards are tracked for a user during the time of their deposit and then subtracted from the rewards they get when their share decreases.

Since virtualRewardsToRemove are subtracted from a user's rewards, then a smaller reward virtualRewardsToRemove value is in favor of the user.

We can see this error in this code snippet:

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

Therefore users get more rewards than they are entitled to. If all users try to withdraw, there wouldn't be enough rewards to pay out the last withdrawer.

Tools Used

Manual Review

Recommended Mitigation Steps

The correct rounding direction for all calculations of a users virtual rewards is to round up. Use divCeil so virtualRewardsToRemove rounds up.

Assessed type

Under/Overflow

Lines of code

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

Vulnerability details

Impact

The minimum collateral is enforced to prevent a well known vulnerability - there is no incentive to liquidate small loans. Therefore the impact of this check being bypassed is the same - small loans will not be liquidated which can lead to bad debt in the protocol.

Proof of Concept

The minimum collateral is enforced in the when borrowing USDS, during the call to the maxBorrowableUSDS function. However this check is not applied when the user withdraws collateral. Therefore, a loan with small collateral can be created by:

1. depositing collateral greater than the minimum collateral
2. taking out a small loan
3. withdrawing collateral so that the position is now under the minimum collateral threshold.

Why small loans are an issue:

The liquidation reward is based off a flat 5% of the collateral being liquidated. There is no minimum cap to the liquidation fee.

Let's have an example where gas costs are $5. The collateral being liquidated needs to be $100 USDS or more for the liquidation costs to be worth the reward.

However, a loan that is worth $100 at liquidation is actually worth far more than that upon time of opening. For example, opened at the minimum collateral factor of 200%, the loan is actually worth slightly less than $200 when it was opened, and then decreased to $100. With even lower-leverage positions with 400% CF, it is reasonable that position sizes are $400 or more upon time of opening.

Tools Used

Manual Review

Recommended Mitigation Steps

The minimum collateral should be enforced on withdrawals whenever a user has an active USDS loan.

Assessed type

Invalid Validation

Lines of code

https://github.com/code-423n4/2024-01-salty/blob/53516c2cdfdfacb662cdea6417c52f23c94d5b5b/src/rewards/Emissions.sol#L55

Vulnerability details

Impact

Stakers get slightly less weekly rewards than they should when performUpkeep is called multiple times in a week.

Proof of Concept

The SALT rewards emitted are correct when Upkeep is called exactly every week. However the Upkeep function can be called by anybody and can be called multiple times a week.

When upkeep is called multiple times during a week, the first call distributes the correct amount of rewards (corresponding to 0.5% of rewards balance since start of the week). However, subsequent calls now use the reduced balanceOf(salt) meaning that less rewards are distributed than there should be.

An intuitive explanation of this is that the rewards are negatively compounding per call to upkeep. Every subsequent call of upkeep gives less rewards per unit of time.

Tools Used

Manual Review

Recommended Mitigation Steps

The upkeep should distribute rewards based on the balance during the first call of each week to ensure that the rewards remain consistent to match emissionsWeeklyPercent no matter how many times Upkeep is called during the week.

Assessed type

Math

Lines of code

https://github.com/code-423n4/2024-01-salty/blob/53516c2cdfdfacb662cdea6417c52f23c94d5b5b/src/dao/DAO.sol#L316 https://github.com/code-423n4/2024-01-salty/blob/53516c2cdfdfacb662cdea6417c52f23c94d5b5b/src/dao/DAO.sol#L360

Vulnerability details

Impact

formPOL can be vulnerable to a well known liquidity deposit sandwich attack causing loss of funds for the protocol.

Proof of Concept

In formPOL, there are no slippage parameters when calling depositLiquidityAndIncreaseShare

All liquidity deposits in a Uniswap v2 style pool can be frontrun by this sequence of steps:

1. attacker pushes pool away from correct ratio
2. victims liquidity deposit goes through at wrong ratio
3. attacker sells tokens into the new liquidity for a profit

Liquidity withdrawals in a x * y = k pool are actually rarely profitable to frontrun attack (it requires certain edge cases), as the same sequence actually loses money if it is a liquidity withdrawals, which is why withdrawPOL wasn't explicitly mentioned. However, this issue also applies to withdrawPOL and adding slippage protection in that function could be an extra safety measure.

Tools Used

Manual review

Recommended Mitigation Steps

formPOL should have some form of slippage protection. This could be based off a maximum deviation off some other price feed such a Uniswap v3 TWAP or Chainlink pricefeed. If the pool reserves deviates too much from the expected ratio from the price feed, formPOL should revert.

Assessed type

MEV

Lines of code

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

Vulnerability details

Impact

An attacker can inflate the value of liquidity through reserve ratio manipulation (even without manipulating the aggregated oracle) and take out a undercollateralized USDS loan.

Proof of Concept

The value of a liquidity position in Salty is determined by:

1. Checking the reserves of the pool/liquidity position (easy to manipulate)
2. Valuing the tokens that comprise the liquidity position using an aggregated oracle (difficult to manipulate)

The only invariant enforced by the Uniswap v2 style AMM is $x * y = k$. When the ratio of the tokens deviates from the "correct" ratio (where the ratio corresponds exactly to the correct value of the tokens), the liquidity will always be worth more , as calculated by the USD value of the individual tokens. There is the well known frontrunning attack on liquidity deposits which works based on this fact.

Mathematical Example

Let's go through a simple example. The pool is USDT-DAI, which is selected for simplicity as they are both tokens with 18 decimals and the oracle always returns a value of $1 for both tokens.

Currently there is 1e18 tokens in each pool, and let's say that the constant k is 1e36

Invariant formula: $x * y = k$

Let's do a swap which doubles the x variable and recalculate the corresponding y value:

Now if we apply the correct oracle price of the tokens, which is $1 per 1e18 wei, the value of the LP position is now worth (0.5 + 2) = $2.5 , which is more , than the correct value of $2.

An intuitive explanation of value inflation

We know that large trades that push the reserve ratio away from the correct ratio loses money for traders (this money is gained by LP's). Trades that move the reserves towards the correct ratio gain money for traders (denominated by the "real value" of the tokens). This is the basis for AMM arbitrage and slippage attacks.

Pushing the price far from the correct reserve ratio can thus be seen as a temporary inflation of the price of the LP token when the token reserves are valued by their USD denominated price. This inflation comes from the manipulating trader losing a large amount of USD-denominated value.

Using this intuitive analogy, we can see why it's only possible to inflate the LP price and not deflate it. Pushing the reserve ratio towards the correct ratio is the method of deflating the USD-denominated value, however, the non-manipulated value should always approximately match the correct price, so it is not possible!

Escalating to an Attack

The LP token is used as collateral to borrow USDS, so we can inflate the LP price to borrow more than the liquidity position's real worth. The attacker can then dump the USDS and the whole USDS stablecoin will be undercollateralized and collapse.

Does Automated Arbitrage Prevent Pool Manipulation?

There is potential difficulty in manipulating the Salty pool due to the automated arbitrage. If there is enough liquidity in the pools in the arbitragePath, then the protocol will swap WETH to more WETH through the path which will rebalance the pool.

Note that this requires "enough liquidity" in the pool path. One could simply deposit a large amount liquidity in the pool being manipulated such that it holds far more token value than the other pools. Therefore, the WETH arbitrage will leave the pools imbalanced due to causing alot of price slippage in the smaller pools to create only a small price correction in the large pool.

Note that the potential profit of the attacker, which is to borrow all the USDS available at a massively discounted rate, is huge. Therefore the loss of losing to the WETH arbitrage would be negligible compared to how much they gain.

Tools Used

Manual Review

Recommended Mitigation Steps

Valuing LP positions is tricky. Salty's current method is to "trust" the untrustworthy pool reserves. Instead, you could value the liquidity as if the ratio was at the correct ratio, rather than the current pool ratio. Here is an example of a protocol implementing this solution for Uniswap v3 positions:

https://github.com/arcadia-finance/accounts-v2/blob/main/src/asset-modules/UniswapV3/UniswapV3AM.sol

Assessed type

Uniswap

Lines of code

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

Vulnerability details

Impact

The first depositor in the AMM can lose the majority of their tokens to a frontrunning attack even if they set the correct minShares slippage parameters.

Proof of Concept

Summary:

By using $x + y$ instead of $sqrt(x * y)$ as the formula for determining the initial liquidity shares, minshares is no longer a effective slippage parameter for the first deposit as front running can change the shares minted even when the token ratio is the same.

Explanation:

Salty is a Uniswap style pool which maintains an $x * y = k$ invaraint. However, the equation determining the initial liquidity amount is different from Uniswap v2

Here is Uniswap's:

Here is Salty's (implemented in the _addLiquidity function):

Here is a quote from Uniswap's whitepaper:

"Uniswap v2 initially mints shares equal to the geometric mean of the amounts, liquidity = sqrt(xy). This formula ensures that the value of a liquidity pool share at any time is essentially independent of the ratio at which liquidity was initially deposited… The above formula ensures that a liquidity pool share will never be worth less than the geometric mean of the reserves in that pool."

Uniswap's liquidity formula is path-independent. As long as we know the x and y values in a deposit, we know that the liquidity will conform to the $L = sqrt(x * y)$ formula.

Salty's formula is not path independent. The number of shares minted for a token deposit is affected by the initial liquidity deposit.

This is the basis for this attack.

The only slippage parameter used for depositing into the AMM is minShares. However, during the first deposit, an attacker can exploit the difference in the initial deposit liquidity equation $x + y$ and the invariant $x * y$ to inflate the number of shares minted per token, and then use the inflated shares to set the AMM at an incorrect reserve ratio such that it can be arbitraged.

First let's examine a base case, where a user makes an initial deposit with correct minShares parameters:

User deposits 10000 DAI and 10000 USDT at a 1-1 reserve ratio. This is correct as they are both tokens with 18 decimals and value of $1. Let's calculate the minShares slippage parameters they should set. During the first deposit they are depositing 10000 * 1e18 wei of each token, so 1e22 wei. Liquidity minted is the sum of the 2 amounts (1e22 + 1e22). So liquidity minted = 2e22

Now let's examine how a frontrunner can inflate the shares so that the slippage parameter is no longer effective.

Frontrunning Case, Step by step:

1. Attacker deposits at very imbalanced token ratios, eg 1e2 USDT and 1e18 DAI. This is to make the (x + y) result, or the liquidity shares, inflated relative to the correct deposit ratio.
2. This sets x * y = 1e20
3. So now we manipulate the x * y back to be equal
4. x = 1e10 and y = 1e10
5. 1e18 shares represents only 1e10 and 1e10 tokens
6. If the user deposits now, they get far more shares. So the user gets 1e8 (10 million) times more shares per wei of token deposited.
7. Depositing 1e22 of each token now yields 1e30 shares! In the example without frontrunning, they only minted 1e22 shares
8. The example so far involves manipulating the AMM back to the "correct" reserve ratio to mint >1 million times more shares. However, this won't make any profit to the attacker yet. The attacker can use the "extra shares" to make the victims transaction go through even if they set "correct" minShares parameters based on the non-frontrunned case. So therefore, they manipulate the exchange-rate where 1 DAI = 5 USDT, and then even though less shares are minted, the extra shares minted from the attacker's share inflation ensures the shares minted does not drop below the minShares parameter.

Tools Used

Manual Review

Recommended Mitigation Steps

Consider using minAmount0 and minAmount1 deposited into the AMM. This is how Uniswap V3 implements their slippage parameters.

Alternatively, using the same liquidity formula as Uniswap v2 - $L = sqrt(x * y)$ will prevent this attack.

Assessed type

MEV

Lines of code

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

Vulnerability details

Impact

The amount swapped in a zap call is not accurately calculated, leading to less liquidity being deposited and extra slippage loss incurred by users.

Proof of Concept

During the Zap where tokens are swapped, the _determineZapSwapAmount function calculates the exact number of tokens to swap to balance the amounts in to match the reserve ratio after the swap.

However, this makes an incorrect assumption that after the swap takes place, the liquidity deposit happens with no changes to the AMM pool occuring in between. However, the protocol actually does the in-protocol arbitrage during the swap, and this changes the reserve ratio of the underlying pool:

The call chain to the arbitrage is:

_dualZapInLiquidity -> depositSwapWithdraw -> _adjustReservesForSwapAndAttemptArbitrage

Tools Used

Manual Review

Recommended Mitigation Steps

The liquidity deposit should take into account the protocol arbitrage that will take place during the swap to calculate the correct amount of tokens to swap.

Assessed type

Math