It does not check if the pool has ever been whitelisted before when whitelisting it

[c4-bot-8]

Lines of code

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

Vulnerability details

Impact

A pool can receive far more than 200k SALT if it is whitelisted/unwhitelisted multiple times

Proof of Concept

When a pool is ready to be whitelisted, it will receive bootstrapping rewards of 200K SALT. However, it doesn't check if the pool has ever been whitelisted before. If somehow one pool has chance be whitelisted second time, it will receive 200K more SALT bootstrapping rewards, which should not be allowed.

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

  function testGetDoubleRewardDistributionAfterSecondWhitelisting() 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();
    uint256 initialDaoBalance = salt.balanceOf(address(dao));

    dao.finalizeBallot(ballotID);

    // Assert that the token has been whitelisted
    assertTrue(poolsConfig.tokenHasBeenWhitelisted(test, wbtc, weth), "Token was not whitelisted");

    // Assert the correct amount of bootstrapping rewards have been deducted
    uint256 finalDaoBalance = salt.balanceOf(address(dao));
    assertEq(finalDaoBalance, initialDaoBalance - (bootstrapRewards * 2), "Bootstrapping rewards were not correctly deducted");

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

    ballotID = proposals.proposeTokenWhitelisting(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 whitelisted");
    //@audit-info totaly double bootstrapping rewards was sent to whitelisted pool
    finalDaoBalance = salt.balanceOf(address(dao));
    assertEq(finalDaoBalance, initialDaoBalance - (bootstrapRewards * 4));

    vm.stopPrank();
  }

Tools Used

Manual review

Recommended Mitigation Steps

• Introduce a mapping variable to record whether one pool has received bootstrapping rewards

• Check if a pool is eligible to receive the bootstrapping rewards before whitelist it:

• mapping(bytes32=>bool) public hasBeenRewarded; function _finalizeTokenWhitelisting( uint256 ballotID ) internal { if ( proposals.ballotIsApproved(ballotID ) ) { // The ballot is approved. Any reversions below will allow the ballot to be attemped to be finalized later - as the ballot won't be finalized on reversion. Ballot memory ballot = proposals.ballotForID(ballotID);

  uint256 bootstrappingRewards = daoConfig.bootstrappingRewards();

  // Make sure that the DAO contract holds the required amount of SALT for bootstrappingRewards. // Twice the bootstrapping rewards are needed (for both the token/WBTC and token/WETH pools) uint256 saltBalance = exchangeConfig.salt().balanceOf( address(this) );

• require( saltBalance >= bootstrappingRewards * 2, "Whitelisting is not currently possible due to insufficient bootstrapping rewards" );

  // Fail to whitelist for now if this isn't the whitelisting proposal with the most votes - can try again later. uint256 bestWhitelistingBallotID = proposals.tokenWhitelistingBallotWithTheMostVotes(); require( bestWhitelistingBallotID == ballotID, "Only the token whitelisting ballot with the most votes can be finalized" );

  // All tokens are paired with both WBTC and WETH, so whitelist both pairings poolsConfig.whitelistPool( pools, IERC20(ballot.address1), exchangeConfig.wbtc() ); poolsConfig.whitelistPool( pools, IERC20(ballot.address1), exchangeConfig.weth() );

  bytes32 pool1 = PoolUtils._poolID( IERC20(ballot.address1), exchangeConfig.wbtc() ); bytes32 pool2 = PoolUtils._poolID( IERC20(ballot.address1), exchangeConfig.weth() );

  // Send the initial bootstrappingRewards to promote initial liquidity on these two newly whitelisted pools AddedReward[] memory addedRewards = new AddedReward;

• addedRewards[0] = AddedReward( pool1, bootstrappingRewards );

• addedRewards[1] = AddedReward( pool2, bootstrappingRewards );

• if (!hasBeenRewarded[pool1]) {

• hasBeenRewarded[pool1] = true;

• addedRewards[0] = AddedReward( pool1, bootstrappingRewards );

• } else {

• addedRewards[0] = AddedReward( pool1, 0 );

• }

• if (!hasBeenRewarded[pool2]) {

• hasBeenRewarded[pool2] = true;

• addedRewards[1] = AddedReward( pool2, bootstrappingRewards );

• } else {

• addedRewards[1] = AddedReward( pool2, 0 );

• }

• uint totalRewards = addedRewards[0].amountToAdd + addedRewards[1].amountToAdd;

• require( saltBalance >= totalRewards, "Whitelisting is not currently possible due to insufficient bootstrapping rewards" );

• exchangeConfig.salt().approve( address(liquidityRewardsEmitter), bootstrappingRewards * 2 );

• exchangeConfig.salt().approve( address(liquidityRewardsEmitter), totalRewards ); liquidityRewardsEmitter.addSALTRewards( addedRewards );

  emit WhitelistToken(IERC20(ballot.address1)); }

  // Mark the ballot as finalized (which will also remove it from the list of open token whitelisting proposals) proposals.markBallotAsFinalized(ballotID); }

Assessed type

Other

[c4-judge]

Picodes marked the issue as duplicate of #823

[c4-judge]

Picodes marked the issue as unsatisfactory: Invalid

[piken]

This issue is not the dup of #823

823 assumes that one token can be proposed multi times before it is whitelisted, which has been disputed by the sponsor.

However, the root cause in #702 is that a token can be whitelisted again after it was unwhitelisted, and this could cause the problem. Below is a simple attack scenario:

• TokenA is whitelisted first time:
  • 200K SALT bootstrapping rewards is sent to the pool of [WBTC, TokenA]
  • 200K SALT bootstrapping rewards is sent to the pool of [WETH, TokenA]
• TokenA is unwhitelisted, No any SALT is withdrawn from the pools: [WBTC, TokenA], [WETH, TokenA]
• TokenA is whitelisted again,. Since there is no way to check if bootstrapping rewards has been sent before:
  • another 200K SALT bootstrapping rewards is sent to the pool of [WBTC, TokenA]
  • another 200K SALT bootstrapping rewards is sent to the pool of [WETH, TokenA]

The pools of [WBTC, TokenA] and [WETH, TokenA] got unfair bootstrapping rewards.

The PoC in #702 can prove that the issue exists.

[Picodes]

@piken Assuming the pool was unwhitelisted, and LPs are left, why not send bootstrapping rewards a second time to attract initial liquidity a second time? It would be a suitable design as well.

[piken]

@Picodes Thanks for replying my comment.

I have seen nowhere that it is an intentional design to send bootstrapping rewards a second time when a pool is second whitelisted. On the contrary, it's clearly stated that each pool should only receive 200k SALT as Bootstrapping Rewards. in Salty doc:

Bootstrapping Rewards (200k SALT per pool*) - established when a pool is whitelisted.

If such a design were intentional, it could allow liquidity providers to earn double or more rewards from a pool which is whitelisted two or more times, potentially disadvantaging providers in other pools. This scenario might incentivize a SALT holder to repeatedly whitelist a single pool, maximizing their rewards and draining the DAO of Bootstrapping Rewards unfairly.

Could we please involve @othernet-global to review this issue? Based on the comment they left on #823, it seems they may share the view that allowing one pool to receive multiple Bootstrapping Rewards should not be an intentional design choice.

Thank you a lot!

[othernet-global]

It will be up to the DAO to whitelist the pool for the second time. The DAO will be aware that whitelisting will provide bootstrapping rewards again and allowing them to choose so is by design.

[Picodes]

@piken " established when a pool is whitelisted" doesn't clearly say that this should happen only once. This is at most an instance of "function incorrect as to spec" so of Low severity.

[c4-judge]

Picodes marked the issue as unsatisfactory: Invalid