Salty.IO - 0xCiphky's results

Lines of code

https://github.com/code-423n4/2024-01-salty/blob/53516c2cdfdfacb662cdea6417c52f23c94d5b5b/src/staking/StakingRewards.sol#L57 https://github.com/code-423n4/2024-01-salty/blob/53516c2cdfdfacb662cdea6417c52f23c94d5b5b/src/staking/StakingRewards.sol#L147 https://github.com/code-423n4/2024-01-salty/blob/53516c2cdfdfacb662cdea6417c52f23c94d5b5b/src/staking/StakingRewards.sol#L232 https://github.com/code-423n4/2024-01-salty/blob/53516c2cdfdfacb662cdea6417c52f23c94d5b5b/src/Upkeep.sol#L244

Vulnerability details

Vulnerability Details:

Liquidity providers in the protocol earn rewards for their contributions. These rewards are updated each time a user's share changes or they claim rewards. The _increaseUserShare function recalculates and assigns rewards whenever a user increases their stake.

function _increaseUserShare(address wallet, bytes32 poolID, uint256 increaseShareAmount, bool useCooldown)
        internal
    {
        ...
        uint256 existingTotalShares = totalShares[poolID];
        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);
        }
        ...
    }

The protocol uses the virtualRewards variable to account for any rewards before the user, this amount will be deducted when a user claims their rewards or decreases their stake.

Here, virtualRewardsToAdd is computed using the formula:

• virtualRewardsToAdd = (totalRewards[poolID] x increaseShareAmount) x existingTotalShares

This computation can disproportionately affect early LPs, especially when their added amount is comparable to or larger than the existing total shares, leading to inflated virtual rewards and potentially denying them rewards for an extended period.

Example Scenario:

Consider the following:

The protocol can start with up to 5.5k rewards for every LP pool, that is the 1% daily emission. The upkeep function is responsible for transferring up to the maximum allowable daily rewards to the staking contract.

• The upkeep contract employs a timer to regulate the frequency and quantity of rewards distribution. However, since this timer begins counting from the moment of the contract's deployment (in the constructor), and considering the initial voting period for starting up the exchange spans several days, it becomes feasible to distribute the maximum daily reward amount by invoking upkeep.

User 1 adds liquidity, receiving 5 shares.

• User1 won't have virtual rewards calculated, as existingTotalShares would be zero.

User 2 follows, adding 10 shares. The virtualRewardsToAdd for User 2 would be:

• virtualRewardsToAdd = (totalRewards[poolID] * increaseShareAmount) / existingTotalShares
  • (5.5k * 10) / 5 = 11k.
  • This results in User 2 having 11k virtual rewards, despite the pool only containing 5.5k actual rewards.

Assume after several hours, additional users have joined, diluting User 2's share in the pool.

• User2 share = 10
• totalShares = 100
• totalRewards = 5.5k(initial) + user virtual(11k) + other user virtual(80.5k) added rewards(3k) = 100k

User 2 decides to claim their rewards through the claimAllRewards function, which internally utilizes the userRewardForPool function.

• The formula for User 2's reward share calculation:
  • rewardsShare = (totalRewards[poolID] * user.userShare) / totalShares[poolID]
  • (100k * 10) / 100 = 10k

Here, even though the total rewards in the pool have increased to 100k, indicating that rewards have been earned, User 2's share of the rewards (10k) is still less than their inflated virtual rewards total of 11k.

• As a result, the userRewardForPool function will compute a zero reward for User 2, despite the increase in total rewards (from upkeep). This discrepancy can lead to User 2, and potentially other users in similar positions, not receiving their due rewards.

Impact:

Early LPs will have their virtual rewards inflated, leading to a long duration where they cannot claim any rewards.

Proof Of Concept

function testInflatedVirtualRewards() public {
    bytes32 poolID1 = PoolUtils._poolID( wbtc, weth );
    bytes32[] memory poolIDs = new bytes32[](1);
    poolIDs[0] = poolID1;
    skip(2 days);
    uint256 depositedWBTC = ( 1 ether *10**8) / priceAggregator.getPriceBTC();
    uint256 depositedWETH = ( 1 ether *10**18) / priceAggregator.getPriceETH(); 
    (uint256 reserveWBTC, uint256 reserveWETH) = pools.getPoolReserves(wbtc, weth);
    upkeep.performUpkeep();
    // check total rewards of pool
    uint256[] memory rewardsbefore = new uint256[](1);
    rewardsbefore = collateralAndLiquidity.totalRewardsForPools(poolIDs);
    // Alice deposit collateral (first user)
    vm.startPrank(alice);
    collateralAndLiquidity.depositCollateralAndIncreaseShare( depositedWBTC, depositedWETH, 0, block.timestamp, false );
    vm.stopPrank();
    // bob deposit 10 times alice collateral
    _readyUser(bob);
    vm.startPrank(bob);
    collateralAndLiquidity.depositCollateralAndIncreaseShare( depositedWBTC * 10, depositedWETH * 10, 0, block.timestamp, false );
    vm.stopPrank();
    // check bobs virtual rewards
    uint virtualRewardsBob = collateralAndLiquidity.userVirtualRewardsForPool(bob, poolIDs[0]);
    // bobs virtual rewards is greater then all initial pool rewards
    assertTrue(virtualRewardsBob > rewardsbefore[0]);
    }

Tools Used:

• Manual analysis
• Foundry

Recommendation:

Invoking the performUpkeep function just before the initial distribution can help mitigate this issue. This action resets the timer, ensuring that rewards do not start at the maximum daily amount, thus preventing the overinflation of virtual rewards for early LPs.

Assessed type

Other

Lines of code

https://github.com/code-423n4/2024-01-salty/blob/53516c2cdfdfacb662cdea6417c52f23c94d5b5b/src/staking/Staking.sol#L41 https://github.com/code-423n4/2024-01-salty/blob/53516c2cdfdfacb662cdea6417c52f23c94d5b5b/src/staking/Staking.sol#L130 ****https://github.com/code-423n4/2024-01-salty/blob/53516c2cdfdfacb662cdea6417c52f23c94d5b5b/src/staking/StakingRewards.sol#L57 https://github.com/code-423n4/2024-01-salty/blob/53516c2cdfdfacb662cdea6417c52f23c94d5b5b/src/staking/StakingRewards.sol#L232 https://github.com/code-423n4/2024-01-salty/blob/53516c2cdfdfacb662cdea6417c52f23c94d5b5b/src/launch/Airdrop.sol#L74 https://github.com/code-423n4/2024-01-salty/blob/53516c2cdfdfacb662cdea6417c52f23c94d5b5b/src/staking/Staking.sol#L60 https://github.com/code-423n4/2024-01-salty/blob/53516c2cdfdfacb662cdea6417c52f23c94d5b5b/src/Upkeep.sol#L244 https://github.com/code-423n4/2024-01-salty/blob/53516c2cdfdfacb662cdea6417c52f23c94d5b5b/src/staking/Staking.sol#L84 https://github.com/code-423n4/2024-01-salty/blob/53516c2cdfdfacb662cdea6417c52f23c94d5b5b/src/staking/StakingRewards.sol#L147

Vulnerability details

Summary:

The protocol's launch includes an airdrop feature where whitelisted users can claim and stake SALT after the conclusion of the BootstrapBallot. However, in the current implementation a vulnerability exists where the first user can claim all available SALT rewards in the staking contract.

Vulnerability Details:

The claimAirdrop function in the Airdrop contract allows airdrop participants to claim the airdrop. This function stakes SALT (stakeSALT) with its own address first and then unstake’s and stakes it with the user’s address (transferStakedSaltFromAirdropToUser).

function claimAirdrop() external nonReentrant {
        require(claimingAllowed, "Claiming is not allowed yet");
        require(isAuthorized(msg.sender), "Wallet is not authorized for airdrop");
        require(!claimed[msg.sender], "Wallet already claimed the airdrop");

        // Have the Airdrop contract stake a specified amount of SALT and then transfer it to the user
        staking.stakeSALT(saltAmountForEachUser);
        staking.transferStakedSaltFromAirdropToUser(msg.sender, saltAmountForEachUser);

        claimed[msg.sender] = true;
    }

The staking functionality calls the _increaseUserShare function to calculate a user's share in the pool. The issue arises here, particularly in how it deals with the virtualRewards calculation for the first user. Since there is no current shares in the pool, then the virtualRewards calculation is skipped.

// Increase a user's share for the given whitelisted pool.
    function _increaseUserShare(address wallet, bytes32 poolID, uint256 increaseShareAmount, bool useCooldown)
        internal
    {
        ...
	uint256 existingTotalShares = totalShares[poolID];

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

To understand the implications of this, we need to look at how a user's rewards are calculated. The formula used in the userRewardForPool function is:

• uint256 rewardsShare = (totalRewards[poolID] * user.userShare) / totalShares[poolID];

From this calculated rewardsShare, virtualRewards are then deducted:

• return rewardsShare - user.virtualRewards;

In the case where the first user stakes in an empty pool, they end up having the same number of shares as the totalShares in the pool, but with zero virtualRewards. This means that the first user can claim all the SALT rewards in the staking contract, as their share of rewards would not have the necessary deduction of virtualRewards.

// Returns the user's pending rewards for a specified pool.
    function userRewardForPool(address wallet, bytes32 poolID) public view returns (uint256) {
        ...
        // Determine the share of the rewards for the user based on their deposited share
        uint256 rewardsShare = (totalRewards[poolID] * user.userShare) / totalShares[poolID];
	...
        return rewardsShare - user.virtualRewards;
    }

To exploit this vulnerability, two key issues must be bypassed:

1. Lack of Initial Rewards in the Contract:
   • Initially, the staking contract does not contain any rewards, meaning that if a user were to claim rewards immediately, they would receive nothing. To overcome this, the user needs to trigger the upkeep function. This function is responsible for transferring up to the maximum allowable daily rewards to the staking contract.
     • The upkeep contract employs a timer to regulate the frequency and quantity of rewards distribution. However, since this timer begins counting from the moment of the contract's deployment (in the constructor), and considering the initial airdrop voting period spans several days, it becomes feasible to distribute the maximum daily reward amount by invoking upkeep.
2. Behaviour of the Airdrop Claim Function:
   • The existing airdrop claim function stakes and then unstake’s the initial airdrop amount using the Airdrop contract's address before staking under the user’s address. This means that if the upkeep function is called before the airdrop claim, the rewards will be credited to the Airdrop contract, not the user. This is a separate issue. To solve this, the user must first claim the airdrop, with zero rewards in the contract, then follow the sequence below.

Thus, the execution sequence to exploit this vulnerability would be:

• Claim airdrop (airdrop.claimAirdrop())
• Fully unstake (staking.unstake(initialXSaltBalance, 52))
• Call upkeep (upkeep.performUpkeep)
• Cancel unstake (staking.cancelUnstake(unstakeID))
• Claim all rewards (staking.claimAllRewards(poolIDs))

Impact:

A whitelisted user can exploit this vulnerability to claim all the current salt staking rewards in the contract. Initially, there are 3 million bootstrap rewards in the stakingRewardsEmitter, which are emitted at a rate of 1% per day. As a result, the first airdrop recipient could claim up to 30k SALT.

Proof Of Concept

function testClaimAllDailyRewards() external {
        // increase time by 2 days
        skip(2 days);
        bytes32[] memory poolIDs = new bytes32[](1);
        poolIDs[0] = PoolUtils.STAKED_SALT;
        // Whitelist
        whitelistAlice();
        // Approve the distribution to allow claiming
        vm.prank(address(bootstrapBallot));
        initialDistribution.distributionApproved();
        assertTrue(airdrop.claimingAllowed(), "Claiming should be allowed for the test");
        // update excluded countries
        vm.prank(address(dao));
        accessManager.excludedCountriesUpdated();
        // Claims airdrop
        vm.prank(alice);
        airdrop.claimAirdrop();
        // Alice fully unstake
        uint256 initialXSaltBalance = staking.userXSalt(alice);
        vm.prank(alice);
        (uint256 unstakeID) = staking.unstake(initialXSaltBalance, 52);
        uint256[] memory rewardsBefore = staking.totalRewardsForPools(poolIDs);
        uint256 totalRewardsBefore = rewardsBefore[0];
        // call upkeeps
        vm.prank(alice);
        upkeep.performUpkeep();
        // check total rewards in contract
        uint256[] memory rewards = staking.totalRewardsForPools(poolIDs);
        uint256 totalRewards = rewards[0];
        // Alice cancel unstake
        vm.prank(alice);
        staking.cancelUnstake(unstakeID);
        // alice claims all rewards in contract
        vm.prank(alice);
        // claim all rewards
        (uint256 claimableRewards) = staking.claimAllRewards(poolIDs);
    
        assertEq(claimableRewards, totalRewards);
        assertEq(salt.balanceOf(alice), totalRewards);
    }

Tools Used:

• Manual analysis
• Foundry

Recommendation:

The protocol can address this vulnerability in two ways:

• Call the performUpkeep function just before the initial distribution. This resets the timer, ensuring that a very small amount of rewards is sent to the staking contract if called again.
• Change the lastUpkeepTime to the start of when the exchange goes live, instead of in the constructor. This also ensures that only a minimal amount of rewards is sent to the staking contract upon subsequent calls, mitigating the problem.

Assessed type

Other

Lines of code

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

Vulnerability details

Vulnerability Details:

Liquidity providers can add liquidity to the protocol using the depositCollateralAndIncreaseShare or depositLiquidityAndIncreaseShare functions, both functions call the _increaseUserShare function to stake the users liquidity and account for the positions rewards. The current implementation has an issue, particularly in how it deals with the virtualRewards calculation for the first user. Since there is no current shares in the pool, then the virtualRewards calculation is skipped.

// Increase a user's share for the given whitelisted pool.
    function _increaseUserShare(address wallet, bytes32 poolID, uint256 increaseShareAmount, bool useCooldown)
        internal
    {
        ...
	uint256 existingTotalShares = totalShares[poolID];

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

To understand the implications of this, we need to look at how a user's rewards are calculated. The formula used in the userRewardForPool function is:

• uint256 rewardsShare = (totalRewards[poolID] * user.userShare) / totalShares[poolID];

From this calculated rewardsShare, virtualRewards are then deducted:

• return rewardsShare - user.virtualRewards;

In the case where the first user stakes in an empty pool, they end up having the same number of shares as the totalShares in the pool, but with zero virtualRewards. This means that the first user can claim all the pools rewards in the staking contract, as their share of rewards would not have the necessary deduction of virtualRewards.

// Returns the user's pending rewards for a specified pool.
    function userRewardForPool(address wallet, bytes32 poolID) public view returns (uint256) {
        ...
        // Determine the share of the rewards for the user based on their deposited share
        uint256 rewardsShare = (totalRewards[poolID] * user.userShare) / totalShares[poolID];
	...
        return rewardsShare - user.virtualRewards;
    }

A potential issue is the lack of Initial Rewards in the Contract. Initially, the staking contract does not contain any rewards, meaning that if a user were to claim rewards immediately, they would receive nothing. To overcome this, the user needs to trigger the upkeep function. This function is responsible for transferring up to the maximum allowable daily rewards to the staking contract.

The upkeep contract employs a timer to regulate the frequency and quantity of rewards distribution. However, since this timer begins counting from the moment of the contract's deployment (in the constructor), and considering the initial voting period for starting up the exchange spans several days, it becomes feasible to distribute the maximum daily reward amount by invoking upkeep.

Impact:

A LP can exploit this vulnerability to claim all the current staking rewards in the contract. Initially, there are 555k SALT bootstrap rewards per pool in the stakingRewardsEmitter, which are emitted at a rate of 1% per day. As a result, the first LP could claim up to 5.5k SALT.

Proof Of Concept

function testFirstLPCanClaimAllRewards() public {
        assertEq(salt.balanceOf(alice), 0);
        bytes32 poolID1 = PoolUtils._poolID( wbtc, weth );
        bytes32[] memory poolIDs = new bytes32[](1);
        poolIDs[0] = poolID1;
        skip(2 days);
        // 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);
	vm.startPrank(alice);
        // Alice call upkeep
        upkeep.performUpkeep();
        // check total rewards for pool
        uint256[] memory totalRewards = new uint256[](1);
        totalRewards = collateralAndLiquidity.totalRewardsForPools(poolIDs);
        // Alice will deposit collateral 
	(uint256 addedAmountWBTC, uint256 addedAmountWETH, uint256 addedLiquidity) = collateralAndLiquidity.depositCollateralAndIncreaseShare( depositedWBTC, depositedWETH, 0, block.timestamp, false );
        // check alices rewards
        uint rewardsAlice = collateralAndLiquidity.userRewardForPool(alice, poolIDs[0]);
        collateralAndLiquidity.claimAllRewards(poolIDs);
        vm.stopPrank();

        assertEq(totalRewards[0], rewardsAlice);
        assertEq(salt.balanceOf(alice), totalRewards[0]);
    }

Tools Used:

• Manual analysis
• Foundry

Recommendation:

The protocol can address this vulnerability in two ways:

• Call the performUpkeep function just before the initial distribution. This resets the timer, ensuring that a very small amount of rewards is sent to the staking contract if called again.
• Change the lastUpkeepTime to the start of when the exchange goes live, instead of in the constructor. This also ensures that only a minimal amount of rewards is sent to the staking contract upon subsequent calls, mitigating the problem.

Assessed type

Other

Lines of code

https://github.com/code-423n4/2024-01-salty/blob/53516c2cdfdfacb662cdea6417c52f23c94d5b5b/src/stable/CollateralAndLiquidity.sol#L140 https://github.com/code-423n4/2024-01-salty/blob/53516c2cdfdfacb662cdea6417c52f23c94d5b5b/src/staking/StakingRewards.sol#L97 https://github.com/code-423n4/2024-01-salty/blob/53516c2cdfdfacb662cdea6417c52f23c94d5b5b/src/staking/StakingRewards.sol#L57

Vulnerability details

Summary:

The Protocol's liquidateUser function is designed to handle positions that fall under the minimum collateral ratio. This function transfers out the position's collateral to cover the debt and sets the liquidated user's share to zero. However, the method used to decrease a user's share incorporates a timer that limits the frequency of share adjustments. This timer can be exploited by users facing liquidation, allowing them to block the liquidation process.

Vulnerability Details:

The liquidateUser function interacts with the _decreaseUserShare function in the StakingRewards contract to reset a user's shares upon liquidation.

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

The issue arises from the cooldown feature within _decreaseUserShare, which restricts the frequency of modifying a user's position. When the cooldown is active, it blocks the liquidation process, even if the user's position is eligible for liquidation based on their collateral status. The liquidateUser function currently sets useCooldown to true, thereby enabling this cooldown mechanism.

function _decreaseUserShare(address wallet, bytes32 poolID, uint256 decreaseShareAmount, bool useCooldown)
        internal
    {
        ...
        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();
            }
        }
        ...
    }

Impact

• Users can exploit this mechanism to prevent their positions from being liquidated by front running a liquidate call with a minimal increase in their share, resetting the cooldown timer. This can be repeated, effectively blocking the liquidation for as long as the user wants at minimal cost.
• Additionally, even non-malicious users might inadvertently trigger this issue, leading to undercollateralized positions that cannot be liquidated.

This situation will lead to the protocol incurring debt from undercollateralized positions, thus affecting its overall stability.

Proof Of Concept

function testBlockLiquidation() public {
    // 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);
    // Alice will deposit collateral and borrow max USDS
    vm.startPrank(alice);
    collateralAndLiquidity.depositCollateralAndIncreaseShare( depositedWBTC, depositedWETH, 0, block.timestamp, false );
    // Deposit again
    vm.warp( block.timestamp + 1 hours );
    collateralAndLiquidity.depositCollateralAndIncreaseShare( depositedWBTC, depositedWETH, 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.startPrank(alice);
    // alice borrow max again
    vm.warp( block.timestamp + 1 hours);
    uint maxUSDS = collateralAndLiquidity.maxBorrowableUSDS(alice);
    collateralAndLiquidity.borrowUSDS( maxUSDS );
    vm.stopPrank();
    // Artificially crash the collateral price
    _crashCollateralPrice();
    // Delay before the liquidation
    vm.warp( block.timestamp + 1 days );
    // Alice front run liquidation with minimum deposit to block the liquidation
    vm.prank(alice);
    collateralAndLiquidity.depositCollateralAndIncreaseShare( 1e4, 1e4, 0, block.timestamp, false );
    // check if alice can be liquidated
    (bool canLiquidate) = collateralAndLiquidity.canUserBeLiquidated(alice);
    assertEq(canLiquidate, true);
    // Liquidate Alice's position will fail
    vm.startPrank(bob);
    vm.expectRevert("Must wait for the cooldown to expire");
    collateralAndLiquidity.liquidateUser(alice);
    vm.stopPrank();
}

Tools Used:

• Manual analysis
• Foundry

Recommendation:

By setting the useCooldown parameter to false in the _decreaseUserShare function call within liquidateUser, liquidations will no longer be hindered by the cooldown period. This change will prevent the exploitation of the cooldown feature to avoid liquidation.

function liquidateUser(address wallet) external nonReentrant {
        ...
        // Decrease the user's share of collateral as it has been liquidated and they no longer have it.
        _decreaseUserShare(wallet, collateralPoolID, userCollateralAmount, false);
        ...
    }

Assessed type

DoS

Lines of code

https://github.com/code-423n4/2024-01-salty/blob/53516c2cdfdfacb662cdea6417c52f23c94d5b5b/src/stable/CollateralAndLiquidity.sol#L80 https://github.com/code-423n4/2024-01-salty/blob/53516c2cdfdfacb662cdea6417c52f23c94d5b5b/src/staking/Liquidity.sol#L157 https://github.com/code-423n4/2024-01-salty/blob/53516c2cdfdfacb662cdea6417c52f23c94d5b5b/src/stable/CollateralAndLiquidity.sol#L140 https://github.com/code-423n4/2024-01-salty/blob/53516c2cdfdfacb662cdea6417c52f23c94d5b5b/src/pools/Pools.sol#L170

Vulnerability details

Summary:

The protocol's current mechanism for liquidity withdrawal does not allow the last liquidity provider left or only LP in a pool to fully withdraw their stake or be liquidated due to a reserve threshold constraint.

Vulnerability Details:

LPs withdraw liquidity using withdrawCollateralAndClaim or withdrawLiquidityAndClaim functions, both relying on removeLiquidity. The removeLiquidity function includes a requirement preventing reserve balances from dropping below PoolUtils.DUST (100). This restriction implies that the initial or final LP can't fully withdraw their liquidity or rewards. While the amount will be negligible for tokens like ETH (0.0000000000000001) with 18 decimal places, it could be more substantial for tokens with fewer decimals.

function removeLiquidity(
        ...
    ) external nonReentrant returns (uint256 reclaimedA, uint256 reclaimedB) {
        ...
        require(
            (reserves.reserve0 >= PoolUtils.DUST) && (reserves.reserve0 >= PoolUtils.DUST),
            "Insufficient reserves after liquidity removal"
        );
	...
    }

More importantly, the liquidateUser function utilized for liquidating undercollateralized positions withdraws all of a user's liquidity. In scenarios where a user is the sole LP, the PoolUtils.DUST constraint prevents a liquidation even if a position is undercollateralized.

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

While this edge case is unlikely, it should still be handled by the protocol as it can not only lead to a loss of user funds but also be used to abuse the liquidation system and block a position from being liquidated.

Impact:

This issue restricts the last LP left or only LP in a pool from fully withdrawing their liquidity or rewards, leading to a loss of funds. It also presents a loophole in the liquidation process.

Proof Of Concept

function testLPCantFW() public {
    bytes32 poolID1 = PoolUtils._poolID( wbtc, weth );
    bytes32[] memory poolIDs = new bytes32[](1);
    poolIDs[0] = poolID1;
    skip(2 days);
    // 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);
    vm.startPrank(alice);
    // Alice will deposit collateral 
    (uint256 addedAmountWBTC, uint256 addedAmountWETH, uint256 addedLiquidity) = collateralAndLiquidity.depositCollateralAndIncreaseShare( depositedWBTC, depositedWETH, 0, block.timestamp, false );
    // Alice tries to remove all collateral
    vm.expectRevert("Insufficient reserves after liquidity removal");
    collateralAndLiquidity.withdrawCollateralAndClaim(addedLiquidity ,0,0,block.timestamp);
    vm.stopPrank();
    }

Tools Used:

• Manual analysis
• Foundry

Recommendation:

The team should deposit a minimal amount of liquidity (slightly above the PoolUtils.DUST threshold) to all current and future whitelisted pools. This ensures that the reserve threshold does not hinder full withdrawal or liquidation processes.

Assessed type

Other

Lines of code

https://github.com/code-423n4/2024-01-salty/blob/53516c2cdfdfacb662cdea6417c52f23c94d5b5b/src/rewards/SaltRewards.sol#L117 https://github.com/code-423n4/2024-01-salty/blob/53516c2cdfdfacb662cdea6417c52f23c94d5b5b/src/rewards/RewardsEmitter.sol#L57 https://github.com/code-423n4/2024-01-salty/blob/53516c2cdfdfacb662cdea6417c52f23c94d5b5b/src/rewards/RewardsEmitter.sol#L82

Vulnerability details

Summary:

Liquidity providers in the protocol are rewarded from various sources, including SALT Emissions, arbitrage profits, and initial bootstrapping rewards. The distribution of these rewards depends on each pool's contribution to generating arbitrage profits. However, an issue arises when a pool is removed from the whitelist through the DAO's decision, leading to the permanent locking of its pending rewards.

Vulnerability Details:

The SaltRewards contract temporarily holds SALT rewards from emissions and arbitrage profits. These rewards are then meant to be distributed to the stakingRewardsEmitter and liquidityRewardsEmitter. The distribution to the latter is based on each pool's share in recent arbitrage profits.

function performUpkeep( bytes32[] calldata poolIDs, uint256[] calldata profitsForPools ) external
        {
	     ....
	     // Divide up the remaining rewards between SALT stakers and liquidity providers
	     uint256 stakingRewardsAmount = ( remainingRewards * rewardsConfig.stakingRewardsPercent() ) / 100;
	     uint256 liquidityRewardsAmount = remainingRewards - stakingRewardsAmount;

	     _sendStakingRewards(stakingRewardsAmount);
             _sendLiquidityRewards(liquidityRewardsAmount, directRewardsForSaltUSDS, poolIDs, profitsForPools, totalProfits);
        }

In the RewardsEmitter contract, rewards are tracked within the pendingRewards mapping for each pool.

function addSALTRewards(AddedReward[] calldata addedRewards) external nonReentrant {
        ...
            uint256 amountToAdd = addedReward.amountToAdd;
            if (amountToAdd != 0) {
                // Update pendingRewards so the SALT can be distributed later
                pendingRewards[addedReward.poolID] += amountToAdd;
                sum += amountToAdd;
        ...
    }

The performUpkeep function in the RewardsEmitter contract is responsible for the daily distribution of rewards, releasing a percentage (1% per day). The problem is when a pool is removed from the whitelist, it invariably leaves behind a portion of pending rewards within the contract. This remainder is due to the contract's design of emitting only a fixed daily percentage.

Since the performUpkeep function uses the poolsConfig.whitelistedPools function for determining eligible pools for reward distribution. Once a pool is no longer whitelisted, it becomes excluded from this distribution process. The contract also lacks a mechanism to either redistribute these pending rewards to other pools or to transfer them out leaving them locked in the contract permanently.

function performUpkeep(uint256 timeSinceLastUpkeep) external {
        ...

        if (isForCollateralAndLiquidity) {
            // For the liquidityRewardsEmitter, all pools can receive rewards
            poolIDs = poolsConfig.whitelistedPools();
    
	...
        // Add the rewards so that they can later be claimed by the users proportional to their share of the StakingRewards derived contract.
        stakingRewards.addSALTRewards(addedRewards);
    }

Impact:

This issue leads to the permanent locking of all pending rewards for any pool removed from the whitelist. These rewards include the remainder of the initial Bootstrapping Rewards, pending SALT Emissions and arbitrage profits.

Tools Used:

• Manual analysis

Recommendation:

Update to the RewardsEmitter contract, introducing a function that either reallocates the pending rewards of delisted pools to active ones or enables their transfer to a specified reserve.

Assessed type

Other

Lines of code

https://github.com/code-423n4/2024-01-salty/blob/53516c2cdfdfacb662cdea6417c52f23c94d5b5b/src/dao/Proposals.sol#L81 https://github.com/code-423n4/2024-01-salty/blob/53516c2cdfdfacb662cdea6417c52f23c94d5b5b/src/dao/Proposals.sol#L196 https://github.com/code-423n4/2024-01-salty/blob/53516c2cdfdfacb662cdea6417c52f23c94d5b5b/src/dao/Proposals.sol#L213 https://github.com/code-423n4/2024-01-salty/blob/53516c2cdfdfacb662cdea6417c52f23c94d5b5b/src/dao/Proposals.sol#L240 https://github.com/code-423n4/2024-01-salty/blob/53516c2cdfdfacb662cdea6417c52f23c94d5b5b/src/dao/DAO.sol#L131 https://github.com/code-423n4/2024-01-salty/blob/53516c2cdfdfacb662cdea6417c52f23c94d5b5b/src/dao/DAO.sol#L155 https://github.com/code-423n4/2024-01-salty/blob/53516c2cdfdfacb662cdea6417c52f23c94d5b5b/src/dao/Proposals.sol#L249

Vulnerability details

Summary:

The Proposals contract in the protocol enables SALT stakers to propose and vote on diverse ballots. However, a problem exists where the naming system used in proposals can be exploited to disrupt DAO operations.

Vulnerability Details:

The issue stems from the _possiblyCreateProposal function, which checks if a proposal with a specific name is already open. This mechanism is intended to prevent duplicate proposals but can be misused to block DAO functionalities.

function _possiblyCreateProposal(
        string memory ballotName,
        BallotType ballotType,
        address address1,
        uint256 number1,
        string memory string1,
        string memory string2
    ) internal returns (uint256 ballotID) {
        ...
        // Make sure that a proposal of the same name is not already open for the ballot
        require(openBallotsByName[ballotName] == 0, "Cannot create a proposal similar to a ballot that is still open");
        ...
        emit ProposalCreated(ballotID, ballotType, ballotName);
    }

Lets look at each function that can be exploited and the consequences.

proposeSendSALT

• The proposeSendSALT function uses a fixed ballot name, "sendSALT", which restricts the number of active proposals for sending SALT to just one up to the ballotMinimumDuration (defaulted to 10 days).
• A malicious user can create a spam proposal, such as proposing to send a negligible amount of SALT (e.g., 1). This will block any legitimate proposals from being created until the ballotMinimumDuration expires.
• Moreover, the simplicity and negligible cost of creating such spam proposals mean that a malicious user could repeatedly exploit this vulnerability, continually locking up the function for consecutive 10-day periods.

function proposeSendSALT(address wallet, uint256 amount, string calldata description)
        external
        nonReentrant
        returns (uint256 ballotID)
    {
        ...
        string memory ballotName = "sendSALT";
        return _possiblyCreateProposal(ballotName, BallotType.SEND_SALT, wallet, amount, "", description);
    }

proposeCallContract

• The proposeCallContract function in the DAO is designed for proposing calls to any arbitrary contract. The ballotName, is constructed by concatenating a preset string with the contract's address.
• A malicious user could front run a proposal using the same contract address but with a different number. This effectively blocks any genuine proposal intended for that contract address.

// Proposes calling the callFromDAO(uint256) function on an arbitrary contract.
    function proposeCallContract(address contractAddress, uint256 number, string calldata description)
        external
        nonReentrant
        returns (uint256 ballotID)
    {
        require(contractAddress != address(0), "Contract address cannot be address(0)");

        string memory ballotName = string.concat("callContract:", Strings.toHexString(address(contractAddress)));
        return _possiblyCreateProposal(ballotName, BallotType.CALL_CONTRACT, contractAddress, number, description, "");
    }

proposeSetContractAddress

• The proposeSetContractAddress function within the DAO is intended for proposing new addresses for critical components such as price feeds or the access manager contract.

function proposeSetContractAddress(string calldata contractName, address newAddress, string calldata description)
        external
        nonReentrant
        returns (uint256 ballotID)
    {
        require(newAddress != address(0), "Proposed address cannot be address(0)");

        string memory ballotName = string.concat("setContract:", contractName);
        return _possiblyCreateProposal(ballotName, BallotType.SET_CONTRACT, newAddress, 0, "", description);
    }

• This function generates a ballot name using a predetermined string followed by the contractName. In regards to the contractName, the _executeSetContract function looks for names corresponding to numbered price feed or the term "accessManager."
• A malicious user can front run a legitimate proposal by creating a proposal with the same contract name paired with an invalid address, this will mean no one else can create a proposal for at least ten days for that particular price feed or the access manager.

function _executeSetContract( Ballot memory ballot ) internal
   {
       bytes32 nameHash = keccak256(bytes( ballot.ballotName ) );

	if ( nameHash == keccak256(bytes( "setContract:priceFeed1_confirm" )) )
		priceAggregator.setPriceFeed( 1, IPriceFeed(ballot.address1) );
	else if ( nameHash == keccak256(bytes( "setContract:priceFeed2_confirm" )) )
	        priceAggregator.setPriceFeed( 2, IPriceFeed(ballot.address1) );
	else if ( nameHash == keccak256(bytes( "setContract:priceFeed3_confirm" )) )
	        priceAggregator.setPriceFeed( 3, IPriceFeed(ballot.address1) );
	else if ( nameHash == keccak256(bytes( "setContract:accessManager_confirm" )) )
		exchangeConfig.setAccessManager( IAccessManager(ballot.address1) );

	emit SetContract(ballot.ballotName, ballot.address1);
   }

proposeSetContractAddress and proposeWebsiteUpdate confirmations (look at POC test)

• The proposeSetContractAddress and proposeWebsiteUpdate functions incorporate an additional step of initiating a second confirmation ballot. This is activated once the initial proposal passes, serving as a safeguard against last-minute approvals.
• The creation of this confirmation ballot is managed by the createConfirmationProposal function, which also employs the _possiblyCreateProposal method.
• While this safety net protects from last minute approvals, it can be abused the following way:
  • User A submits a proposal to set a contract address.
  • Anticipating the initiation of a confirmation ballot upon approval of User A's proposal, User B creates a new proposal with the identical name as User A’s, but append "_confirm" to it.
  • When User A’s original proposal passes and attempts to trigger the confirmation ballot (_executeApproval), the process fails.
• This strategy effectively obstructs the confirmation process in the DAO, capable of halting all related confirmation proposals for a duration of at least ten days.

function _executeApproval( Ballot memory ballot ) internal 
   {
		...
		// Once an initial setContract proposal passes, it automatically starts a second confirmation ballot (to prevent last minute approvals)
		else if ( ballot.ballotType == BallotType.SET_CONTRACT )
		proposals.createConfirmationProposal( string.concat(ballot.ballotName, "_confirm"), BallotType.CONFIRM_SET_CONTRACT, ballot.address1, "", ballot.description );

		// Once an initial setWebsiteURL proposal passes, it automatically starts a second confirmation ballot (to prevent last minute approvals)
		else if ( ballot.ballotType == BallotType.SET_WEBSITE_URL )
		proposals.createConfirmationProposal( string.concat(ballot.ballotName, "_confirm"), BallotType.CONFIRM_SET_WEBSITE_URL, address(0), ballot.string1, ballot.description );
		...
   }

Moreover, the _possiblyCreateProposal function includes a specific check to prevent the creation of a proposal if a confirmation ballot with a similar name already exists.

function _possiblyCreateProposal(
        string memory ballotName,
        ...
    ) internal returns (uint256 ballotID) {
        ...
        require(
            openBallotsByName[string.concat(ballotName, "_confirm")] == 0,
            "Cannot create a proposal for a ballot with a secondary confirmation"
        );

        ...
    }

This check can also be exploited, as regular proposals ending with "_confirm" will block any genuine regular proposals related to them, even if they are not actual confirmation proposals themselves.

Proof Of Concept

function testBlockConfirmationProposal() public {
        // set up the initial state
        // have alice send bob 1e5 ether of salt so bob can stake and propose
        vm.prank(alice);
        salt.transfer(bob, 100000 ether);
        // have bob stake 1e5 ether of salt
        vm.prank(bob);
        staking.stakeSALT(100000 ether);
        // have alice stake 1e5 ether of salt
        vm.startPrank(alice);
        staking.stakeSALT(1000000 ether);
         // Check initial state
        uint256 initialProposalCount = proposals.nextBallotID() - 1;
        // create proposal params
        address newAddress = address(0x1111111111111111111111111111111111111112);
        // propose a new contract address
        proposals.proposeSetContractAddress("contractName", newAddress, "description");
        vm.stopPrank();

        vm.startPrank(DEPLOYER);
        // Check if a new proposal is created
        uint256 newProposalCount = proposals.nextBallotID() - 1;
        // Get the new proposal
        Ballot memory ballot = proposals.ballotForID(newProposalCount);
        vm.stopPrank();

        vm.startPrank(bob);
        // bob creates a new proposal, use same name as alices proposal adding "_confirm" to the end
        // This will block alices proposal from getting finalized
        address newAddressB = address(0x1111111111111111111111111111111111111113);
        proposals.proposeSetContractAddress("contractName_confirm", newAddressB, "description");
        vm.stopPrank();

        // check requiredQuorumForBallotType
        uint256 expectedQuorum = proposals.requiredQuorumForBallotType(BallotType.SET_CONTRACT);
        //check requiredQuorumForBallotType for BallotType.PARAMETER
        uint256 expectedQuorumParam = proposals.requiredQuorumForBallotType(BallotType.PARAMETER);
          //check requiredQuorumForBallotType for BallotType.WHITELIST_TOKEN
        uint256 expectedQuorumWhitelist = proposals.requiredQuorumForBallotType(BallotType.WHITELIST_TOKEN);
        console.log("expectedQuorum: ", expectedQuorum);
        console.log("expectedQuorumParam: ", expectedQuorumParam);
        console.log("expectedQuorumWhitelist: ", expectedQuorumWhitelist);

        vm.prank(bob);
        // bob votes
        proposals.castVote(newProposalCount, Vote.YES);

        // process to get alices vote through
        // alice votes
        vm.startPrank(alice);
        proposals.castVote(newProposalCount, Vote.YES);
        // skip 11 days
        skip(11 days);

        // check totalVotesCastForBallot
        uint256 totalVotes = proposals.totalVotesCastForBallot(newProposalCount);
        console.log("totalVotes: ", totalVotes);

        // check total shares
        uint256 totalShares = staking.totalShares(PoolUtils.STAKED_SALT);
        console.log("totalShares: ", totalShares);

        // finalize the ballot will fail as bobs proposal will have the same name
        vm.expectRevert("Cannot create a proposal similar to a ballot that is still open");
        dao.finalizeBallot(newProposalCount);
    }

Impact:

Various essential functions within the DAO can be obstructed by users for the duration of the ballotMinimumDuration, which could be extended to even longer periods. The different exploits can be carried out repeatedly and without incurring any cost to the malicious users. Consequently, this would prevent the DAO from functioning correctly.

Tools Used:

• Manual analysis
• Foundry

Recommendation:

Addressing this issue requires custom solutions for each affected function. Adjusting the restrictions on proposal naming while still implementing limits on the number of identical proposals could mitigate the risk.

Assessed type

DoS

Lines of code

https://github.com/code-423n4/2024-01-salty/blob/53516c2cdfdfacb662cdea6417c52f23c94d5b5b/src/ManagedWallet.sol#L42 https://github.com/code-423n4/2024-01-salty/blob/53516c2cdfdfacb662cdea6417c52f23c94d5b5b/src/ManagedWallet.sol#L59 https://github.com/code-423n4/2024-01-salty/blob/53516c2cdfdfacb662cdea6417c52f23c94d5b5b/src/ManagedWallet.sol#L73

Vulnerability details

Summary:

The ManagedWallet contract provides two wallet addresses (a main and confirmation wallet) which can be changed using the following mechanism:

1. Main wallet can propose a new main wallet and confirmation wallet.

2. Confirmation wallet confirms or rejects.

3. There is a timelock of 30 days before the proposed mainWallet can confirm the change.

However the current implementation has a loophole where the timelock between the two actions can be bypassed.

Vulnerability Details:

The proposeWallets function allows the main wallet to propose new main and confirmation wallets. These proposed addresses are stored in proposedMainWallet and proposedConfirmationWallet, awaiting confirmation by the current confirmation wallet.

function proposeWallets( address _proposedMainWallet, address _proposedConfirmationWallet ) external
		{
		require( msg.sender == mainWallet, "Only the current mainWallet can propose changes" );
		require( _proposedMainWallet != address(0), "_proposedMainWallet cannot be the zero address" );
		require( _proposedConfirmationWallet != address(0), "_proposedConfirmationWallet cannot be the zero address" );

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

		proposedMainWallet = _proposedMainWallet;
		proposedConfirmationWallet = _proposedConfirmationWallet;

		emit WalletProposal(proposedMainWallet, proposedConfirmationWallet);
		}

The confirmationWallet can confirm or reject a proposal by sending .05 ether to the ManagedWallet contract, this will activate the timelock and set it to the current block.timestamp plus the TIMELOCK_DURATION.

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
        }

Once the TIMELOCK_DURATION has passed the changeWallets function can be called by the proposedMainWallet address to confirm the change and reset all other variables for future proposals.

function changeWallets() external
	{
		// proposedMainWallet calls the function - to make sure it is a valid address.
		require( msg.sender == proposedMainWallet, "Invalid sender" );
		require( block.timestamp >= activeTimelock, "Timelock not yet completed" );

		// Set the wallets
		mainWallet = proposedMainWallet;
		confirmationWallet = proposedConfirmationWallet;

		emit WalletChange(mainWallet, confirmationWallet);

		// Reset
		activeTimelock = type(uint256).max;
		proposedMainWallet = address(0);
		proposedConfirmationWallet = address(0);
	}

The problem is since the proposeWallets function does not influence the activeTimelock, the current confirmationWallet address can perform the following steps to bypass the timelock between the proposal and confirmation.

• confirmationWallet address sends .05 ether to the ManagedWallet contract. (30 days prior)
• mainWallet address calls the proposeWallets function with the proposed addresses.
• proposedMainWallet address can then immediately call the changeWallets function skipping the timelock between the proposal and confirmation.

Impact:

While the main and confirmation wallet addresses will be trusted addresses, the current ManagedWallet contracts process of changing wallets can still be compromised using the method above. The timelock feature is implemented as a safety net by the protocol however in the current implementation it can be skipped and a change can be made immediately which could cause major problems.

Proof Of Concept

function testChangeWalletSkipTimelock() public {
        // Set up the initial state with main and confirmation wallets
        address initialMainWallet = alice;
        address initialConfirmationWallet = address(0x2222);
        ManagedWallet managedWallet = new ManagedWallet(initialMainWallet, initialConfirmationWallet);

        // Set up the proposed main and confirmation wallets
        address newMainWallet = address(0x3333);
        address newConfirmationWallet = address(0x4444);

        // Prank as the current confirmation wallet and send ether to confirm the proposal
        uint256 sentValue = 0.06 ether;
        vm.prank(initialConfirmationWallet);
        vm.deal(initialConfirmationWallet, sentValue);
        (bool success,) = address(managedWallet).call{value: sentValue}("");
        assertTrue(success, "Confirmation of wallet proposal failed");

        // Warp the blockchain time to the future beyond the active timelock period
        uint256 currentTime = block.timestamp;
        vm.warp(currentTime + TIMELOCK_DURATION);

        // Prank as the initial main wallet to propose the new wallets
        vm.startPrank(initialMainWallet);
        managedWallet.proposeWallets(newMainWallet, newConfirmationWallet);
        vm.stopPrank();

        // Prank as the new proposed main wallet which should now be allowed to call changeWallets
        vm.prank(newMainWallet);
        managedWallet.changeWallets();

        // Check that the mainWallet and confirmationWallet state variables are updated
        assertEq(managedWallet.mainWallet(), newMainWallet, "mainWallet was not updated correctly");
        assertEq(managedWallet.confirmationWallet(), newConfirmationWallet, "confirmationWallet was not updated correctly");
    }

Tools Used:

• Manual analysis
• Foundry

Recommendation:

The problem above can be avoided by resetting the activeTimelock to type(uint256).max in the proposeWallets function, that way when the TIMELOCK_DURATION can’t be skipped once a proposal is made.

function proposeWallets( address _proposedMainWallet, address _proposedConfirmationWallet ) external
		{
		require( msg.sender == mainWallet, "Only the current mainWallet can propose changes" );
		require( _proposedMainWallet != address(0), "_proposedMainWallet cannot be the zero address" );
		require( _proposedConfirmationWallet != address(0), "_proposedConfirmationWallet cannot be the zero address" );

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

		proposedMainWallet = _proposedMainWallet;
		proposedConfirmationWallet = _proposedConfirmationWallet;
                activeTimelock = type(uint256).max; //add here

		emit WalletProposal(proposedMainWallet, proposedConfirmationWallet);
		}

Assessed type

Other

Lines of code

https://github.com/code-423n4/2024-01-salty/blob/53516c2cdfdfacb662cdea6417c52f23c94d5b5b/src/staking/Liquidity.sol#L83 https://github.com/code-423n4/2024-01-salty/blob/53516c2cdfdfacb662cdea6417c52f23c94d5b5b/src/staking/Liquidity.sol#L146 https://github.com/code-423n4/2024-01-salty/blob/53516c2cdfdfacb662cdea6417c52f23c94d5b5b/src/staking/Liquidity.sol#L50 https://github.com/code-423n4/2024-01-salty/blob/53516c2cdfdfacb662cdea6417c52f23c94d5b5b/src/pools/PoolMath.sol#L152 https://github.com/code-423n4/2024-01-salty/blob/53516c2cdfdfacb662cdea6417c52f23c94d5b5b/src/pools/PoolMath.sol#L212

Vulnerability details

Summary:

The protocol features a zapping function that balances token amounts by swapping one token for another if needed before adding liquidity. This ensures that the tokens are in a ratio equivalent to the reserves, maximizing returns for the user. While generally effective, an issue arises when a user's provided amount exceeds the reserves. In such cases, the function caps the swap at the reserve level and proceeds to add liquidity at an imbalanced ratio, leading to the user receiving an undervalued amount.

Vulnerability Details:

The _dualZapInLiquidity function calls the _determineZapSwapAmount function to determine the amount to be swapped, the excess token is then swapped to get the ratio as close to the reserves as possible. The _determineZapSwapAmount will first check which amount is in excess and then calls the _zapSwapAmount function with the correct order to determine the swap amount.

function _determineZapSwapAmount( uint256 reserveA, uint256 reserveB, uint256 zapAmountA, uint256 zapAmountB ) public pure returns (uint256 swapAmountA, uint256 swapAmountB )
		{
		// zapAmountA / zapAmountB exceeds the ratio of reserveA / reserveB? - meaning too much zapAmountA
		if ( zapAmountA * reserveB > reserveA * zapAmountB )
			(swapAmountA, swapAmountB) = (_zapSwapAmount( reserveA, reserveB, zapAmountA, zapAmountB ), 0);

		// zapAmountA / zapAmountB is less than the ratio of reserveA / reserveB? - meaning too much zapAmountB
		if ( zapAmountA * reserveB < reserveA * zapAmountB )
			(swapAmountA, swapAmountB) = (0, _zapSwapAmount( reserveB, reserveA, zapAmountB, zapAmountA ));

		// Ensure we are not swapping more than was specified for zapping
		if ( ( swapAmountA > zapAmountA ) || ( swapAmountB > zapAmountB ) )
			return (0, 0);

		return (swapAmountA, swapAmountB);
		}

The _zapSwapAmount function determines swap amounts based on the reserves and the amounts to be zapped. However, if the user's provided amounts exceed the reserves, it limits the swap to the reserve level, leading to the addition of liquidity at an imbalanced ratio.

While the feature is intended to balance token ratios by swapping excess tokens, it will always fail to do so in this scenario. Consequently, early users under the assumption that the zapping feature will automatically balance uneven liquidity, might provide token amounts disproportionately.

Example Scenario:

• Reserves: reserve0 = 1 ether, reserve1 = 1 ether
• User's Deposit: zapAmountA = 10 ether, zapAmountB = 20 ether
• Determined Swap Amounts from zapping function: swapAmountA = 0 ether, swapAmountB = 0.38 ether
• Resulting in an added liquidity ratio of 10.38 ether : 19.62 ether instead of an optimal close to even ratio.

Impact:

Users trying to deposit liquidity early on will face issues:

• Transactions will go through at an uneven rate, netting them a non-optimal ratio and potentially in the best case refunding some tokens.
  • These remaining tokens cannot be redeposited until the cooldown timer on deposits expires, which can be up to six hours.
• Users' transactions may frequently revert as the imbalance will net them fewer tokens than the minimum liquidity set.

Proof Of Concept

function testAddLiqUseZapping() public {
        // Create two new tokens
        vm.startPrank(address(collateralAndLiquidity));
        IERC20 tokenA = new TestERC20("TEST", 18);
        IERC20 tokenB = new TestERC20("TEST", 18);
        tokenA.transfer(alice, 100 ether);
        tokenB.transfer(alice, 100 ether);
        tokenA.transfer(bob, 100 ether);
        tokenB.transfer(bob, 100 ether);
        vm.stopPrank();

        vm.prank(address(dao));
        poolsConfig.whitelistPool(pools, tokenA, tokenB);

        vm.startPrank(address(collateralAndLiquidity));
        tokenA.approve(address(pools), type(uint256).max);
        tokenB.approve(address(pools), type(uint256).max);
        vm.stopPrank();
        // bob add 1ether:1ether liquidity
        vm.startPrank(bob);
        tokenA.approve(address(collateralAndLiquidity), type(uint256).max);
        tokenB.approve(address(collateralAndLiquidity), type(uint256).max);
        collateralAndLiquidity.depositLiquidityAndIncreaseShare(tokenA, tokenB, 1 ether, 1 ether, 0, block.timestamp, true);
        vm.stopPrank();
        // Get the new reserves after adding liquidity
        (bytes32 poolID, bool flipped) = PoolUtils._poolIDAndFlipped(tokenA, tokenB);
        (uint256 reservesA, uint256 reservesB) = pools.getPoolReserves(tokenA, tokenB);
        // Assert that liquidity was added correctly 
        assertEq(reservesA, 1 ether, "Reserve for tokenA did not increase correctly.");
        assertEq(reservesB, 1 ether, "Reserve for tokenB did not increase correctly.");
        // Alice add uneven liquidity using zapping
        vm.startPrank(alice);
        tokenA.approve(address(collateralAndLiquidity), type(uint256).max);
        tokenB.approve(address(collateralAndLiquidity), type(uint256).max);
        collateralAndLiquidity.depositLiquidityAndIncreaseShare(tokenA, tokenB, 10 ether, 20 ether, 0, block.timestamp, true);
        vm.stopPrank();

        (uint256 reservesAafter, uint256 reservesBafter) = pools.getPoolReserves(tokenA, tokenB);
        // Assert that zapping went through andliquidity was added unevenly
        assertTrue(reservesAafter <= 1.2e19, "Reserve for tokenA did not increase evenly.");
        assertTrue(reservesBafter >= 2e19, "Reserve for tokenB did not increase evenly.");
    }

Tools Used:

• Manual analysis
• Foundry

Recommendation:

The protocol should adjust the deposit function to prevent the zapping feature from being used when the amount being added exceeds the reserves. This approach will prevent early users from adding liquidity at suboptimal rates, particularly when they set a loose minimum amount with the expectation that the zapping feature will even it out.

Assessed type

Other