Salty.IO - 0xpiken's results

Lines of code

https://github.com/code-423n4/2024-01-salty/blob/main/src/Upkeep.sol#L231-L239

Vulnerability details

Impact

The Development Team could potentially incur a loss on their SALT distribution reward due to the absence of access control on VestingWallet#release().

Proof of Concept

When Salty exchange is actived, 10M SALT will be transferred to teamVestingWallet by calling InitialDistribution#distributionApproved():

62: 	salt.safeTransfer( address(teamVestingWallet), 10 * MILLION_ETHER );

teamVestingWallet is responsible for distributing 10M SALT linely over 10 years (Deployment.sol#L100):

    teamVestingWallet = new VestingWallet( address(upkeep), uint64(block.timestamp), 60 * 60 * 24 * 365 * 10 );

From the above code we can see that the beneficiary of teamVestingWallet is Upkeep.

Each time Upkeep#performUpkeep() is called, teamVestingWallet will release a certain amount of SALT to Upkeep, the beneficiary, and then the relased SALT will be transferred to mainWallet of managedTeamWallet:

  function step11() public onlySameContract
  {
    uint256 releaseableAmount = VestingWallet(payable(exchangeConfig.teamVestingWallet())).releasable(address(salt));
    
    // teamVestingWallet actually sends the vested SALT to this contract - which will then need to be sent to the active teamWallet
    VestingWallet(payable(exchangeConfig.teamVestingWallet())).release(address(salt));
    
    salt.safeTransfer( exchangeConfig.managedTeamWallet().mainWallet(), releaseableAmount );
  }

However, there is no access control on teamVestingWallet.release(). Any one can call release() to distribute SALT without informing upkeep. upkeep doesn't know how many SALT has been distributed in advance, it has no way to transfer it to the development team, and the distributed SALT by directly calling teamVestingWallet.release() will be locked in upkeep forever.

Copy below codes to DAO.t.sol and run COVERAGE="yes" NETWORK="sep" forge test -vv --rpc-url RPC_URL --match-test testTeamRewardIsLockedInUpkeep

  function testTeamRewardIsLockedInUpkeep() public {
    uint releasableAmount = teamVestingWallet.releasable(address(salt));
    uint upKeepBalance = salt.balanceOf(address(upkeep));
    uint mainWalletBalance = salt.balanceOf(address(managedTeamWallet.mainWallet()));
    //@audit-info a certain amount of SALT is releasable
    assertTrue(releasableAmount != 0);
    //@audit-info there is no SALT in upkeep
    assertEq(upKeepBalance, 0);
    //@audit-info there is no SALT in mainWallet
    assertEq(mainWalletBalance, 0);
    //@audit-info call release() before performUpkeep()
    teamVestingWallet.release(address(salt));
    upkeep.performUpkeep();
    
    upKeepBalance = salt.balanceOf(address(upkeep));
    mainWalletBalance = salt.balanceOf(address(managedTeamWallet.mainWallet()));
    //@audit-info all released SALT is locked in upKeep
    assertEq(upKeepBalance, releasableAmount);
    //@audit-info development team receive nothing
    assertEq(mainWalletBalance, 0);
  }

Tools Used

Manual review

Recommended Mitigation Steps

• Since exchangeConfig.managedTeamWallet is immutable, it is reasonable to config managedTeamWallet as the beneficiary when deploying teamVestingWallet:

-   teamVestingWallet = new VestingWallet( address(upkeep), uint64(block.timestamp), 60 * 60 * 24 * 365 * 10 );
+   teamVestingWallet = new VestingWallet( address(managedTeamWallet), uint64(block.timestamp), 60 * 60 * 24 * 365 * 10 );

• Introduce a new function in managedTeamWallet to transfer all SALT balance to mainWallet:

  function release(address token) external {
    uint balance = IERC20(token).balanceOf(address(this));
    if (balance != 0) {
      IERC20(token).safeTransfer(mainWallet, balance);
    }
  }

• call managedTeamWallet#release() in Upkeep#performUpkeep():

  function step11() public onlySameContract
  {
-   uint256 releaseableAmount = VestingWallet(payable(exchangeConfig.teamVestingWallet())).releasable(address(salt));
    
-   // teamVestingWallet actually sends the vested SALT to this contract - which will then need to be sent to the active teamWallet
    VestingWallet(payable(exchangeConfig.teamVestingWallet())).release(address(salt));
    
-   salt.safeTransfer( exchangeConfig.managedTeamWallet().mainWallet(), releaseableAmount );
+   exchangeConfig.managedTeamWallet().release(address(salt));
  }

Assessed type

Other

Lines of code

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

Vulnerability details

Impact

The collateral sent to liquidizer might be less than expected, leading to a loss for the Salty Protocol.

Proof of Concept

A USDS borrower could be liquidated if their collateral ratio is under the minimum threshold. When liquidating a user by calling CollateralAndLiquidity#liquidateUser(), up to 5%, no more than 500 USD worth of collateral, will be rewarded to the liquidator. (represented by StableConfig.rewardPercentForCallingLiquidation and StableConfig.maxRewardValueForCallingLiquidation, which can be adjusted by DAO)

  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 );
    
    // The caller receives a default 5% of the value of the liquidated collateral.
    uint256 rewardPercent = stableConfig.rewardPercentForCallingLiquidation();
    
    uint256 rewardedWBTC = (reclaimedWBTC * rewardPercent) / 100;
    uint256 rewardedWETH = (reclaimedWETH * rewardPercent) / 100;
    
    // Make sure the value of the rewardAmount is not excessive
    uint256 rewardValue = underlyingTokenValueInUSD( rewardedWBTC, rewardedWETH ); // in 18 decimals
    uint256 maxRewardValue = stableConfig.maxRewardValueForCallingLiquidation(); // 18 decimals
    if ( rewardValue > maxRewardValue )
    {
      rewardedWBTC = (rewardedWBTC * maxRewardValue) / rewardValue;
      rewardedWETH = (rewardedWETH * maxRewardValue) / rewardValue;
    }
    
    // Reward the caller
    wbtc.safeTransfer( msg.sender, rewardedWBTC );
    weth.safeTransfer( msg.sender, rewardedWETH );
    
    // Send the remaining WBTC and WETH to the Liquidizer contract so that the tokens can be converted to USDS and burned (on Liquidizer.performUpkeep)
    wbtc.safeTransfer( address(liquidizer), reclaimedWBTC - rewardedWBTC );
    weth.safeTransfer( address(liquidizer), reclaimedWETH - rewardedWETH );
    
    // Have the Liquidizer contract remember the amount of USDS that will need to be burned.
    uint256 originallyBorrowedUSDS = usdsBorrowedByUsers[wallet];
    liquidizer.incrementBurnableUSDS(originallyBorrowedUSDS);
    
    // Clear the borrowedUSDS for the user who was liquidated so that they can simply keep the USDS they previously borrowed.
    usdsBorrowedByUsers[wallet] = 0;
    _walletsWithBorrowedUSDS.remove(wallet);
    
    emit Liquidation(msg.sender, wallet, reclaimedWBTC, reclaimedWETH, originallyBorrowedUSDS);
  }

Once liquidated, the borrower's share will be decreased by calling _decreaseUserShare():

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

Since useCooldown is true, _decreaseUserShare() will check the cooldown period. It will fail if the cooldown is not expired.

However, if the borrower just increased their collateral ratio by depositing WBTC/WETH in one hour, liquidator is unable to liquidate it at the moment even the collateral ratio of the borrower has dropped far below the minimum threshold, unless the cooldown is expired.

Below is an example that how the protocol suffer a loss under this situation:

• Alice deposits 20000 USD worth of WBTC/WETH
• Alice borrows 10000 USDS, the collateral ratio of alice is 200%
• Alice's collateral is now valued at 12000 USD due to the significant drop in the prices of WBTC/WETH
• Alice deposits an additional 2000 USD worth of WBTC/WETH to mitigate the risk of liquidation.
• 30 minutes later, the value of alice's collateral drops to 10900 USD
• Bob try to liquidate alice but fail due to cooldown restriction.
• the value of alice's collateral drops to 8000 USD after 30 minutes
• Bob liquidates alice and succeed.

If bob can liquidate alice when alice's collateral is valued 10900 USD:

• Bob can receive 500 USD worth of collateral, around 4.59% of alice's collateral
• 10400 USD worth of collteral is sent to liquidizer, which is 95.41% of alice's collateral
• If Liquidizer#performUpkeep() is called in time, the protocol may have chance to repay all alice's USDS debt and keep around 400 USD worth of USDS in Liquidizer However, bob can only liquidate alice when cooldown is expired, alice's collateral is valued only 8000 USD at the moment:
• Bob receives 400 USD worth of collateral, 5% of alice's collateral
• 7600 USD worth of collteral is sent to liquidizer, which is 95% of alice's collateral
• At this time, there is no way to repay all alice's USDS debt with the collateral received from her. The protocol suffers around 2400 USD loss.

Copy below codes to CollateralAndLiquidity.t.sol and run COVERAGE="yes" NETWORK="sep" forge test -vv --rpc-url RPC_URL --match-test testUserLiquidationFailDueToCooldownNotEnd

  function testUserLiquidationFailDueToCooldownNotEnd() public {
    // Have bob deposit so alice can withdraw everything without DUST reserves restriction
    _depositHalfCollateralAndBorrowMax(bob);
    
    // Deposit and borrow for Alice
    uint wbtcBalance = wbtc.balanceOf(alice);
    uint wethBalance = weth.balanceOf(alice);
    vm.startPrank(alice);
    collateralAndLiquidity.depositCollateralAndIncreaseShare(wbtcBalance / 2, wethBalance / 2, 0, block.timestamp, true );
    
    uint256 maxUSDS = collateralAndLiquidity.maxBorrowableUSDS(alice);
    collateralAndLiquidity.borrowUSDS( maxUSDS );
    vm.stopPrank();
    // Check if Alice has a position
    assertTrue(_userHasCollateral(alice));
    
    // Crash the collateral price
    _crashCollateralPrice();
    vm.warp( block.timestamp + 1 days );
    
    vm.prank(alice);
    collateralAndLiquidity.depositCollateralAndIncreaseShare(wbtcBalance / 100, wethBalance / 100, 0, block.timestamp, true );
    //@audit-info alice can not be liquidated
    assertTrue(!collateralAndLiquidity.canUserBeLiquidated(alice));
    // Crash the collateral price
    _crashCollateralPrice();
    //@audit-info alice can be liuqidated now
    assertTrue(collateralAndLiquidity.canUserBeLiquidated(alice));
    //@audit-info however, liquidating alice will fail due to cooldown restriction
    vm.expectRevert("Must wait for the cooldown to expire");
    collateralAndLiquidity.liquidateUser(alice);
  }

Tools Used

Manual review

Recommended Mitigation Steps

useCooldown should be false when liquidating:

  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 );
+   _decreaseUserShare( wallet, collateralPoolID, userCollateralAmount, false );
    
    // The caller receives a default 5% of the value of the liquidated collateral.
    uint256 rewardPercent = stableConfig.rewardPercentForCallingLiquidation();
    
    uint256 rewardedWBTC = (reclaimedWBTC * rewardPercent) / 100;
    uint256 rewardedWETH = (reclaimedWETH * rewardPercent) / 100;
    
    // Make sure the value of the rewardAmount is not excessive
    uint256 rewardValue = underlyingTokenValueInUSD( rewardedWBTC, rewardedWETH ); // in 18 decimals
    uint256 maxRewardValue = stableConfig.maxRewardValueForCallingLiquidation(); // 18 decimals
    if ( rewardValue > maxRewardValue )
    {
      rewardedWBTC = (rewardedWBTC * maxRewardValue) / rewardValue;
      rewardedWETH = (rewardedWETH * maxRewardValue) / rewardValue;
    }
    
    // Reward the caller
    wbtc.safeTransfer( msg.sender, rewardedWBTC );
    weth.safeTransfer( msg.sender, rewardedWETH );
    
    // Send the remaining WBTC and WETH to the Liquidizer contract so that the tokens can be converted to USDS and burned (on Liquidizer.performUpkeep)
    wbtc.safeTransfer( address(liquidizer), reclaimedWBTC - rewardedWBTC );
    weth.safeTransfer( address(liquidizer), reclaimedWETH - rewardedWETH );
    
    // Have the Liquidizer contract remember the amount of USDS that will need to be burned.
    uint256 originallyBorrowedUSDS = usdsBorrowedByUsers[wallet];
    liquidizer.incrementBurnableUSDS(originallyBorrowedUSDS);
    
    // Clear the borrowedUSDS for the user who was liquidated so that they can simply keep the USDS they previously borrowed.
    usdsBorrowedByUsers[wallet] = 0;
    _walletsWithBorrowedUSDS.remove(wallet);
    
    emit Liquidation(msg.sender, wallet, reclaimedWBTC, reclaimedWETH, originallyBorrowedUSDS);
  }

Assessed type

Other

Lines of code

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

Vulnerability details

Impact

The last liquidity provider is blocked to withdraw all their liquidity.

Proof of Concept

Any user can add liquidity into a whitelisted pool by calling Liquidity#depositLiquidityAndIncreaseShare(). The liquidity provider can remove their liquidity at any time by calling Liquidity#withdrawLiquidityAndClaim(). When removing liquidity, A check will be performed to ensure that the reserves of both tokens do not fall below the specified DUST threshold.

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

However, if the last liquidity provider of a whitelisted pool try to remove all their liquidity, it will be blocked due to above inspection. Copy below codes to DAO.t.sol and run COVERAGE="yes" NETWORK="sep" forge test -vv --rpc-url RPC_URL --match-test testRemoveAllLiquidity

  function testRemoveAllLiquidity() public
  {
    vm.startPrank(address(collateralAndLiquidity));
    IERC20 token0 = new TestERC20("TEST", 18);
    IERC20 token1 = new TestERC20("TEST", 6);
    vm.stopPrank();
    
    vm.prank(address(dao));
    poolsConfig.whitelistPool( pools, token0, token1);
    
    vm.startPrank(address(collateralAndLiquidity));
    token0.approve(address(pools), type(uint256).max);
    token1.approve(address(pools), type(uint256).max);
    
    uint256 added0 = 2000 ether;
    uint256 added1 = 1000 * 10 ** 6;
    uint256 liquidityAdded;
    
    (added0, added1, liquidityAdded) = pools.addLiquidity(token0, token1, added0, added1, 0, 0 );
    // Can't remove all the liquidity
    vm.expectRevert( "Insufficient reserves after liquidity removal" );
    pools.removeLiquidity(token0, token1, liquidityAdded, 0, 0, liquidityAdded);
    vm.stopPrank();
  }

Tools Used

Manual review

Recommended Mitigation Steps

Even the last liquidity provider should be allowed to withdraw all of their provided liquidity.

  function removeLiquidity( IERC20 tokenA, IERC20 tokenB, uint256 liquidityToRemove, uint256 minReclaimedA, uint256 minReclaimedB, uint256 totalLiquidity ) external nonReentrant returns (uint256 reclaimedA, uint256 reclaimedB)
  {
    require( msg.sender == address(collateralAndLiquidity), "Pools.removeLiquidity is only callable from the CollateralAndLiquidity contract" );
    require( liquidityToRemove > 0, "The amount of liquidityToRemove cannot be zero" );
    
    (bytes32 poolID, bool flipped) = PoolUtils._poolIDAndFlipped(tokenA, tokenB);
    
    // Determine how much liquidity is being withdrawn and round down in favor of the protocol
    PoolReserves storage reserves = _poolReserves[poolID];
    reclaimedA = ( reserves.reserve0 * liquidityToRemove ) / totalLiquidity;
    reclaimedB = ( reserves.reserve1 * liquidityToRemove ) / totalLiquidity;
    
    reserves.reserve0 -= uint128(reclaimedA);
    reserves.reserve1 -= uint128(reclaimedB);
    
    // Make sure that removing liquidity doesn't drive either of the reserves below DUST.
    // This is to ensure that ratios remain relatively constant even after a maximum withdrawal.
+ if (reserves.reserve0 != 0 || reserves.reserve1 != 0)
    require((reserves.reserve0 >= PoolUtils.DUST) && (reserves.reserve0 >= PoolUtils.DUST), "Insufficient reserves after liquidity removal");
    
    // Switch reclaimed amounts back to the order that was specified in the call arguments so they make sense to the caller
    if (flipped)
      (reclaimedA,reclaimedB) = (reclaimedB,reclaimedA);
    
    require( (reclaimedA >= minReclaimedA) && (reclaimedB >= minReclaimedB), "Insufficient underlying tokens returned" );
    
    // Send the reclaimed tokens to the user
    tokenA.safeTransfer( msg.sender, reclaimedA );
    tokenB.safeTransfer( msg.sender, reclaimedB );
    
    emit LiquidityRemoved(tokenA, tokenB, reclaimedA, reclaimedB, liquidityToRemove);
  }

By fixing Pools#removeLiquidity(), the last collateral provider can also withdraw all their collateral correctly by calling withdrawCollateralAndClaim().

Assessed type

Other

Lines of code

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

Vulnerability details

Impact

There is no way to propose a new main wallet and confirmation wallet if the previous wallet proposal is rejected.

Proof of Concept

The mainWallet of ManagedWallet can propose a new main wallet and confirmation wallet:

  // Make a request to change the main and confirmation wallets.
  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 of ManagedWallet can confirm the proposal by sending a minimum of 0.05 ether to ManagedWallet; otherwise, it 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
  }

However, proposedMainWallet was not set to address(0) when the proposal is rejected. There is no way to create a new proposal unless confirmationWallet confirm the old one. Copy below codes to ManagedWallet.t.sol and run COVERAGE="yes" NETWORK="sep" forge test -vv --rpc-url RPC_URL --match-test testProposeWalletsAfterPreviousProposalIsRejected

  function testProposeWalletsAfterPreviousProposalIsRejected() public {
    ManagedWallet managedWallet = new ManagedWallet(alice, address(this));
    address proposedMainWallet = address(0x3333);
    address proposedConfirmationWallet = address(0x4444);
    
    vm.startPrank(alice);
    managedWallet.proposeWallets(proposedMainWallet, proposedConfirmationWallet);
    vm.stopPrank();
    
    assertEq(managedWallet.proposedMainWallet(), proposedMainWallet, "proposedMainWallet not set correctly");
    assertEq(managedWallet.proposedConfirmationWallet(), proposedConfirmationWallet, "proposedConfirmationWallet not set correctly");
    //@audit-info reject the proposal
    (bool success, ) = address(managedWallet).call{value: 0}("");
    
    //@audit-info activeTimelock was changed to type(uint256).max
    uint256 expectedTimelock = type(uint256).max;
    assertEq(managedWallet.activeTimelock(), expectedTimelock, "activeTimelock should be type(uint256).max");
    assertTrue(success, "Transaction should be successful");
    //@audit-info proposedMainWallet & proposedConfirmationWallet were not reset to address(0)
    assertEq(managedWallet.proposedMainWallet(), proposedMainWallet, "proposedMainWallet was cleaned");
    assertEq(managedWallet.proposedConfirmationWallet(), proposedConfirmationWallet, "proposedConfirmationWallet was cleaned");
    //@audit-info It is impossible to create a new proposal
    vm.expectRevert("Cannot overwrite non-zero proposed mainWallet.");
    vm.prank(alice);
    managedWallet.proposeWallets(proposedMainWallet, proposedConfirmationWallet);
  }

Tools Used

Manual review

Recommended Mitigation Steps

Set proposedMainWallet to address(0) when rejecting the wallet proposal:

  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
+     proposedMainWallet = address(0);
+   }
  }

Assessed type

Other

Lines of code

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

Vulnerability details

Impact

SALT staker might have chance to finalize the vote by manipulating the required quorum

Proof of Concept

When a proposal is created, anyone can vote by calling Proposals#castVote() as far as they have voting power. Anyone can finalize the vote on a specific ballot by calling DAO#finalizeBallot() as soon as the ballot meet below requirements:

• The ballot is live
• ballotMinimumEndTime(the minimum duration) has passed
• The required quorum has been reached

If the total number of votes is less than the required quorum, the ballot can not be finalized:

396:  if ( totalVotesCastForBallot(ballotID) < requiredQuorumForBallotType( ballot.ballotType ))
397:    return false;

The required quorum varies based on the ballot type and the total staked SALT:

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

If a SALT staker unstakes their xSALT, the required quorum will decrease. However, this action does not impact the voting number the staker has casted. A staker might have chance to finalize the ballot in advance by unstaking their xSALT if the total number of votes and required quorum are close enough. Once the ballot is closed, the staker can simply cancel their unstaking without suffering any loss.

Copy below codes to Proposals.t.sol and run COVERAGE="yes" NETWORK="sep" forge test -vv --rpc-url RPC_URL --match-test testCanFinalizeBallotByUnstaking

  function testCanFinalizeBallotByUnstaking() public {
    uint256 initialStake = 10000000 ether;
    vm.prank(DEPLOYER);
    staking.stakeSALT( initialStake );
    
    vm.startPrank(alice);
    staking.stakeSALT(1110111 ether);
    uint256 ballotID = proposals.proposeParameterBallot(2, "description" );
    proposals.castVote(ballotID, Vote.INCREASE);
    vm.stopPrank();
    
    vm.warp(block.timestamp + daoConfig.ballotMinimumDuration() + 1); // ballot end time reached
    
    // Reach quorum
    vm.startPrank(alice);
    //@audit-info before unstaking, the ballot can not be finalized
    bool canFinalizeBeforeUnstaking = proposals.canFinalizeBallot(ballotID);
    assertEq(canFinalizeBeforeUnstaking, false);
    //@audit-info alice unstake SALT
    staking.unstake(1110111 ether, 52);
    //@audit-info after unstaking, the ballot can be finalized now. 
    bool canFinalizeAfterUnstaking = proposals.canFinalizeBallot(ballotID);
    assertEq(canFinalizeAfterUnstaking, true);
    vm.stopPrank();
  }

From above codes we can see, the ballot can be finalized after alice unstakes all her xSALT.

Tools Used

Manual review

Recommended Mitigation Steps

There are several ways to fix this problem. The simplest one is calculating and storing the required quorum when the proposal is created. Once the required quorum is fixed, no one can change it by unstaking their xSALT:

  struct Ballot
  {
    uint256 ballotID;
    bool ballotIsLive;
    
    BallotType ballotType;
    string ballotName;
    address address1;
    uint256 number1;
    string string1;
    string description;
    
    // 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;
+   uint256 requiredQuorum;
  }

  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" );
    }
    
    // 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" );
    require( openBallotsByName[ string.concat(ballotName, "_confirm")] == 0, "Cannot create a proposal for a ballot with a secondary confirmation" );
    
    uint256 ballotMinimumEndTime = block.timestamp + daoConfig.ballotMinimumDuration();
    
    // Add the new Ballot to storage
    ballotID = nextBallotID++;
-   ballots[ballotID] = Ballot( ballotID, true, ballotType, ballotName, address1, number1, string1, string2, ballotMinimumEndTime );
+   uint requiredQuorum = requiredQuorumForBallotType(ballotType);
+   ballots[ballotID] = Ballot( ballotID, true, ballotType, ballotName, address1, number1, string1, string2, ballotMinimumEndTime, requiredQuorum );
    openBallotsByName[ballotName] = ballotID;
    _allOpenBallots.add( ballotID );
    
    // Remember that the user made a proposal
    _userHasActiveProposal[msg.sender] = true;
    _usersThatProposedBallots[ballotID] = msg.sender;
    
    emit ProposalCreated(ballotID, ballotType, ballotName);
  }
  function canFinalizeBallot( uint256 ballotID ) external view returns (bool)
  {
    Ballot memory ballot = ballots[ballotID];
    if ( ! ballot.ballotIsLive )
      return false;
    
    // Check that the minimum duration has passed
    if (block.timestamp < ballot.ballotMinimumEndTime )
      return false;
    
    // Check that the required quorum has been reached
+   if ( totalVotesCastForBallot(ballotID) < requiredQuorumForBallotType( ballot.ballotType ))
-   if ( totalVotesCastForBallot(ballotID) < ballot.requiredQuorum)
      return false;
    
    return true;
  }

Assessed type

Error

Lines of code

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

Vulnerability details

Impact

A proposal could be created to block confirming the passed SET_CONTRACT/SET_WEBSITE_URL proposal.

Proof of Concept

Salty protocol doesn't allow two or more opening proposals owning same name. The protocol also check whether the proposal with name ballotName|_confirm exist to avoid conflict (Otherwise, there is no way to create a secondary confirmation proposal for passed SET_CONTRACT/SET_WEBSITE_URL proposal):

101:    // Make sure that a proposal of the same name is not already open for the ballot
102:    require( openBallotsByName[ballotName] == 0, "Cannot create a proposal similar to a ballot that is still open" );
103:    require( openBallotsByName[ string.concat(ballotName, "_confirm")] == 0, "Cannot create a proposal for a ballot with a secondary confirmation" );

However, it doesn't check if ballotName ends with _confirm. A user who is eligible to create proposal may simply create a proposal to prevent any SET_CONTRACT/SET_WEBSITE_URL proposal to be confirmed.

Copy below codes to DAO.t.sol and run COVERAGE="yes" NETWORK="sep" forge test -vv --rpc-url RPC_URL --match-test testFinalizeSetContractProposalFailed

  function testFinalizeSetContractProposalFailed() public
  {
    vm.prank(DEPLOYER);
    salt.transfer(bob, 1000000 ether );
    
    //@audit-info alice create a SET_CONTRACT proposal, the contractName of which is "priceFeed1"
    vm.startPrank(alice);
    staking.stakeSALT( 1000000 ether );
    uint256 ballotID = proposals.proposeSetContractAddress("priceFeed1", address(0x1231236) , "description" );
    assertEq(proposals.ballotForID(ballotID).ballotIsLive, true, "Ballot not correctly created");
    proposals.castVote(ballotID, Vote.YES);
    vm.stopPrank();
    //@audit-info bob create a SET_CONTRACT proposal, the contractName of which is "priceFeed1_confirm"
    vm.startPrank(bob);
    salt.approve(address(staking), 1000000 ether);
    staking.stakeSALT( 1000000 ether );
    proposals.proposeSetContractAddress("priceFeed1_confirm", address(0x1231236) , "description" );
    vm.stopPrank();
    // Increase block time to finalize the ballot
    vm.warp(block.timestamp + 11 days );
    
    //@audit-info calling finalizeBallot() to finalize alice's proposal. However it is reverted due to ballotName conflition. 
    vm.expectRevert("Cannot create a proposal similar to a ballot that is still open");
    dao.finalizeBallot(ballotID);
  }

Tools Used

Manual review

Recommended Mitigation Steps

• Ensure that contractName doesn't end up with "_confirm" when calling proposeSetContractAddress():
• Ensure that newWebsiteURL doesn't end up with "_confirm" when calling proposeWebsiteUpdate():

+ function _checkProposalName(string calldata name) internal pure {
+   uint nameLength = bytes(name).length;
+   uint confirmLength = bytes("_confirm").length;
+   if (nameLength >= confirmLength) {
+     require(keccak256(abi.encodePacked(name[nameLength - confirmLength:])) != keccak256(abi.encodePacked("_confirm")), "Proposal name can not end up with _confirm");
+   }
+}
  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)" );
+   _checkProposalName(contractName);
    
    string memory ballotName = string.concat("setContract:", contractName );
    return _possiblyCreateProposal( ballotName, BallotType.SET_CONTRACT, newAddress, 0, "", description );
  }
  
  
  function proposeWebsiteUpdate( string calldata newWebsiteURL, string calldata description ) external nonReentrant returns (uint256 ballotID)
  {
    require( keccak256(abi.encodePacked(newWebsiteURL)) != keccak256(abi.encodePacked("")), "newWebsiteURL cannot be empty" );
+   _checkProposalName(newWebsiteURL);
    
    string memory ballotName = string.concat("setURL:", newWebsiteURL );
    return _possiblyCreateProposal( ballotName, BallotType.SET_WEBSITE_URL, address(0), 0, newWebsiteURL, description );
  }

Assessed type

Context

Lines of code

https://github.com/code-423n4/2024-01-salty/blob/main/src/dao/DAO.sol#L278-L291

Vulnerability details

Impact

• The owner of the unclosed proposal can not create new proposal due to the restriction that one user can only has one active proposal
• Other eligible user might not be able to create new proposal due to name conflict

Proof of Concept

An eligible user can create a proposal via different functions, the ballot of which can end as early as ballotMinimumEndTime.
Anyone can vote by calling Proposals#castVote() as far as they have voting power. Anyone can finalize the vote on a specific ballot by calling DAO#finalizeBallot() as soon as the ballot can be finalized.

  function canFinalizeBallot( uint256 ballotID ) external view returns (bool)
  {
    Ballot memory ballot = ballots[ballotID];
    if ( ! ballot.ballotIsLive )
      return false;
    
    // Check that the minimum duration has passed
    if (block.timestamp < ballot.ballotMinimumEndTime )
      return false;
    
    // Check that the required quorum has been reached
@>  if ( totalVotesCastForBallot(ballotID) < requiredQuorumForBallotType( ballot.ballotType ))
      return false;
    
    return true;
  }

As we can see, a ballot can not be finalized if the voting quorum is not reached. Nevertheless, it is common for a ballot to be unable to earn a sufficient number of votes. If such a situation arises, the protocol should offer a method to close the ballot. But there is no such a way to deal with this situation. The only way to close it is to hopefully gather enough votes to meet the quorum. The proposal function could be blocked before it is closed:

• Since one user can only have one active proposal at the same moment, they can not create any new proposal at the moment
• Due to ballot name conflict, other eligible may not able to create new proposal which own the same ballot with unclosed proposal
  • E.g. The protocol enforces the restriction of one sendSALT ballot at a time. No any sendSALT ballot can be created as far as the old one is pending.

Tools Used

Manual review

Recommended Mitigation Steps

• The proposer should be able to cancel their proposal if ballotMinimumEndTime is not ended.
• Anyone can cancel the proposal/ballot without sufficient votes when the time is over ballotMinimumEndTime.

Assessed type

Other

Lines of code

https://github.com/code-423n4/2024-01-salty/blob/main/src/stable/StableConfig.sol#L47-L64

Vulnerability details

Impact

The protocol may not receive the expected amount of collateral when a user is liquidated, posing a risk of the protocol incurring a loss.

Proof of Concept

A USDS borrower could be liquidated if their collateral ratio is under the minimum threshold. The liquidator can receive a propotion of collateral as reward, and the rest collateral will be owned by the protocol.

• The liquidation threshold of collateral ratio is defined by StableConfig.minimumCollateralRatioPercent

• The propotion of collateral reward is defined by StableConfig.rewardPercentForCallingLiquidation

• StableConfig.minimumCollateralRatioPercent can be increased/decreased by calling StableConfig#changeMinimumCollateralRatioPercent()

• StableConfig.rewardPercentForCallingLiquidation can be increased/decreased by calling StableConfig#changeRewardPercentForCallingLiquidation()

No matter how they are changed, the remain collateral ratio after liquidating reward should keep no less than 105% regarding the comment in StableConfig.sol:

L51:

Don't increase rewardPercentForCallingLiquidation if the remainingRatio after the rewards would be less than 105% - to ensure that the position will be liquidatable for more than the originally borrowed USDS amount (assume reasonable market volatility)

L127:

Don't decrease the minimumCollateralRatioPercent if the remainingRatio after the rewards would be less than 105% - to ensure that the position will be liquidatable for more than the originally borrowed USDS amount (assume reasonable market volatility)

However, the calculation was wrong. Even with the default value, the remainingRatio would be less than 105%:

Alice's collateral is valued 8799.99 USD and Alice borrowed 8000 USDS Since the collateral ratio is less than 110%, alice can be liquidated now:

• 5% of collateral(439.99 USD) is rewarded to the liquidator
• The remaining collateral is valued 8360 USD (8799.99 - 439.99)
• remainingRatio is 104.5% (8360 / 8000)

If minimumCollateralRatioPercent is changed to 115 and rewardPercentForCallingLiquidation is changed to 10, the remainingRatio could be dropped to 103.5% (115% - 115% * 10%)

Tools Used

Manual review

Recommended Mitigation Steps

• The max default value of rewardPercentForCallingLiquidation can only be 4 if the default value of minimumCollateralRatioPercent is 110

• Change the calculation method of remainingRatioAfterReward:

  function changeRewardPercentForCallingLiquidation(bool increase) external onlyOwner
  {
    if (increase)
    {
      // Don't increase rewardPercentForCallingLiquidation if the remainingRatio after the rewards would be less than 105% - to ensure that the position will be liquidatable for more than the originally borrowed USDS amount (assume reasonable market volatility)
-     uint256 remainingRatioAfterReward = minimumCollateralRatioPercent - rewardPercentForCallingLiquidation - 1;
+     uint256 remainingRatioAfterReward = minimumCollateralRatioPercent *(100 - rewardPercentForCallingLiquidation - 1)/100;

      if (remainingRatioAfterReward >= 105 && rewardPercentForCallingLiquidation < 10)
        rewardPercentForCallingLiquidation += 1;
    }
    else
    {
    if (rewardPercentForCallingLiquidation > 5)
      rewardPercentForCallingLiquidation -= 1;
    }
    
    emit RewardPercentForCallingLiquidationChanged(rewardPercentForCallingLiquidation);
  }

  function changeMinimumCollateralRatioPercent(bool increase) external onlyOwner
  {
    if (increase)
    {
      if (minimumCollateralRatioPercent < 120)
          minimumCollateralRatioPercent += 1;
    }
    else
    {
      // Don't decrease the minimumCollateralRatioPercent if the remainingRatio after the rewards would be less than 105% - to ensure that the position will be liquidatable for more than the originally borrowed USDS amount (assume reasonable market volatility)
-     uint256 remainingRatioAfterReward = minimumCollateralRatioPercent - 1 - rewardPercentForCallingLiquidation;
+     uint256 remainingRatioAfterReward = (minimumCollateralRatioPercent - 1)*(100 - rewardPercentForCallingLiquidation)/100;
      
      if (remainingRatioAfterReward >= 105 && minimumCollateralRatioPercent > 110)
          minimumCollateralRatioPercent -= 1;
    }
    
    emit MinimumCollateralRatioPercentChanged(minimumCollateralRatioPercent);
  }

Assessed type

Math

Lines of code

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

Vulnerability details

Impact

confirmationWallet can alter their decision after approving/rejecting the wallet proposal.

Proof of Concept

Once mainWallet of ManagedWallet proposes a new main wallet and confirmation wallet, confirmationWallet of ManagedWallet can confirm or reject the proposal by sending ether to ManagedWallet:

  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
  }

However, it doesn't check if the decision has been made before. confirmationWallet could have chance to change the decision without any restriction.

Copy below codes to ManagedWallet.t.sol and run COVERAGE="yes" NETWORK="sep" forge test -vv --rpc-url RPC_URL --match-test testConfirmWalletAlterPreviousDecision

  function testConfirmWalletAlterPreviousDecision() public {
    ManagedWallet managedWallet = new ManagedWallet(alice, address(this));
    address proposedMainWallet = address(0x3333);
    address proposedConfirmationWallet = address(0x4444);
    
    vm.startPrank(alice);
    managedWallet.proposeWallets(proposedMainWallet, proposedConfirmationWallet);
    vm.stopPrank();
    
    assertEq(managedWallet.proposedMainWallet(), proposedMainWallet, "proposedMainWallet not set correctly");
    assertEq(managedWallet.proposedConfirmationWallet(), proposedConfirmationWallet, "proposedConfirmationWallet not set correctly");
    //@audit-info confirm the proposal
    (bool success, ) = address(managedWallet).call{value: 0.05 ether}("");
    
    //@audit-info activeTimelock was changed
    assertTrue(managedWallet.activeTimelock() == block.timestamp + managedWallet.TIMELOCK_DURATION());
    assertTrue(success, "Transaction should be successful");
    //@audit-info refuse the proposal after confirm
    (success, ) = address(managedWallet).call{value: 0}("");
    assertTrue(managedWallet.activeTimelock() == type(uint256).max);
  }

Tools Used

Manual review

Recommended Mitigation Steps

Check if the wallet proposal has been confirmed:

  bool isConfirmed = false;

  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." );
+   isConfirmed = false;    
    proposedMainWallet = _proposedMainWallet;
    proposedConfirmationWallet = _proposedConfirmationWallet;
    
    emit WalletProposal(proposedMainWallet, proposedConfirmationWallet);
  }

  receive() external payable
  {
    require( msg.sender == confirmationWallet, "Invalid sender" );
+   require(!isConfirmed, "The proposal has been confirmed");
    
    // 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
+   isConfirmed = true;
  }

  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);
+   isConfirmed = false;  
  }

Assessed type

Other

Lines of code

https://github.com/code-423n4/2024-01-salty/blob/main/src/dao/DAO.sol#L158-L164

Vulnerability details

Impact

pendingRewards of a pool keeps locked in liquidityRewardsEmitter after the pool is unwhitelisted

Proof of Concept

When a pool is whitelisted, 200K SALT will be rewarded to the pool as bootstrapping rewards. liquidityRewardsEmitter is responsible for distributing the reward timely. Only the whitelisted pool can receive the distributed reward. Once the pool is unwhitelisted, liquidityRewardsEmitter will no longer distribute the reward to it. However, the undistributed reward was not taken back, thereby it will stuck in liquidityRewardsEmitter. The protocol has no way to withdraw the remaining undistributed reward.

Copy below codes to DAO.t.sol and run COVERAGE="yes" NETWORK="sep" forge test -vv --rpc-url RPC_URL --match-test testBootstrapingRewardsStuckInLiquidityRewardsEmitter

  function testBootstrapingRewardsStuckInLiquidityRewardsEmitter() public
  {
    // Alice stakes her SALT to get voting power
    IERC20 test = new TestERC20( "TEST", 18 );
    
    vm.startPrank(address(daoVestingWallet));
    salt.transfer(alice, 1000000 ether);				// for staking and voting
    salt.transfer(address(dao), 1000000 ether); // bootstrapping rewards
    vm.stopPrank();
    
    vm.startPrank(alice);
    staking.stakeSALT(500000 ether);
    
    // Propose a whitelisting ballot
    uint256 ballotID  = proposals.proposeTokenWhitelisting(test, "test", "test");
    proposals.castVote(ballotID, Vote.YES);
    
    // Increase block time to finalize the ballot
    vm.warp(block.timestamp + daoConfig.ballotMinimumDuration());
    
    uint256 bootstrapRewards = daoConfig.bootstrappingRewards();
    
    dao.finalizeBallot(ballotID);
    
    // Assert that the token has been whitelisted
    assertTrue(poolsConfig.tokenHasBeenWhitelisted(test, wbtc, weth), "Token was not whitelisted");
    bytes32[] memory poolIDs = new bytes32[](2);
    poolIDs[0] = PoolUtils._poolID(test,wbtc);
    poolIDs[1] = PoolUtils._poolID(test,weth);
    uint[] memory pendingRewards = liquidityRewardsEmitter.pendingRewardsForPools(poolIDs);
    assertEq(pendingRewards[0], bootstrapRewards);
    assertEq(pendingRewards[1], bootstrapRewards);
    
    uint saltBalanceInEmitterBefore = salt.balanceOf(address(liquidityRewardsEmitter));
    //@audit-info unwhitelisted test token
    ballotID = proposals.proposeTokenUnwhitelisting(test, "test", "test");
    proposals.castVote(ballotID, Vote.YES);
    vm.warp(block.timestamp + daoConfig.ballotMinimumDuration());
    dao.finalizeBallot(ballotID);
    assertTrue(!poolsConfig.tokenHasBeenWhitelisted(test, wbtc, weth), "Token was not unwhitelisted");
    
    pendingRewards = liquidityRewardsEmitter.pendingRewardsForPools(poolIDs);
    //@audit-info pendingRewards of test/wbtc pool and test/weth pool keeped unchanged
    assertEq(pendingRewards[0], bootstrapRewards);
    assertEq(pendingRewards[1], bootstrapRewards);
    uint saltBalanceInEmitterAfter = salt.balanceOf(address(liquidityRewardsEmitter));
    //@audit-info the salt balance of liquidityRewardsEmitter keep unchanged after `test` being unwhitelisted.
    assertEq(saltBalanceInEmitterBefore, saltBalanceInEmitterAfter);
    vm.stopPrank();
  }

Tools Used

Manual review

Recommended Mitigation Steps

Introduce a method that allows the DAO to reclaim undistributed bootstrapping rewards when unwhitelisting a pool.

Assessed type

Other