TempleGold

Vulnerability Details

removeAuctionConfig is used to remove auction config set for last epoch. If the auction was started but the cooldown has not passed yet, both auction config and epoch info are deleted and the _currentEpochId is decremented:

The issue is that _totalAuctionTokenAllocation is not updated in this situation and this will lead to the following consequences for the next epochs:

1. _totalAuctionTokenAllocation will always contain extra auction tokens for all subsequent epochs.

2. Those extra auction tokens can not be recovered.

Consider the following example (that is demonstrated by PoC below) to better explain the consequences:

• For simplicity, we set the auction config for the first epoch with 100 ether auction tokens sent to the Spice contract

• The auction is started, and based on the startAuction logic, the following states are updated as follows:

  • info.totalAuctionTokenAmount = 100 ether:

  uint256 totalAuctionTokenAllocation = _totalAuctionTokenAllocation[auctionToken]; // @audit = 0

  uint256 balance = IERC20(auctionToken).balanceOf(address(this)); // @audit 100 ether

  uint256 epochAuctionTokenAmount = balance - (totalAuctionTokenAllocation - _claimedAuctionTokens[auctionToken]); // @audit 100 ether

  // ...

  info.totalAuctionTokenAmount = epochAuctionTokenAmount; // @audit 100 ether

  • _totalAuctionTokenAllocation[auctionToken] = 100 ether:

  _totalAuctionTokenAllocation[auctionToken] = totalAuctionTokenAllocation + epochAuctionTokenAmount;

• Now, for some reason, the DaoExecutor removes the auction config for the last epoch (the first epoch in our example). Because the epoch is still at cooldown, the following block of removeAuctionConfig will be executed:

  delete auctionConfigs[id];

  delete epochs[id];

  _currentEpochId = id - 1;

  Notice that _totalAuctionTokenAllocation is NOT updated.

• Now, the DaoExecutor wants to set up a new auction (with 100 ether auction token amount) and starts it, so he calls setAuctionConfig and calls startAuction. Normally, we should not feed the Spice contract with 100 ether because the contract already holds it from the previous deleted auction. However, because the _totalAuctionTokenAllocation still holds 100 ether, the epochAuctionTokenAmount will be evaluated to zero, and thus the transaction will revert:

  uint256 totalAuctionTokenAllocation = _totalAuctionTokenAllocation[auctionToken]; // @audit 100 ether

  uint256 balance = IERC20(auctionToken).balanceOf(address(this)); // @audit 100 ether

  uint256 epochAuctionTokenAmount = balance - (totalAuctionTokenAllocation - _claimedAuctionTokens[auctionToken]); // @audit = 0

  ​

  if (epochAuctionTokenAmount < config.minimumDistributedAuctionToken) { revert NotEnoughAuctionTokens(); } // @audit tx will revert

  So, the contract should be fed with an extra 100 ether if we want to start a new auction with 100 ether tokens.

• Now, we fed the Spice contract with the extra 100 ether that we were obliged to, the _totalAuctionTokenAllocation will be updated wrongly:

  uint256 totalAuctionTokenAllocation = _totalAuctionTokenAllocation[auctionToken]; // @audit 100 ether

  uint256 balance = IERC20(auctionToken).balanceOf(address(this)); // @audit 200 ether

  uint256 epochAuctionTokenAmount = balance - (totalAuctionTokenAllocation - _claimedAuctionTokens[auctionToken]); // @audit 100 ether

  // ...

  ​

  info.totalAuctionTokenAmount = epochAuctionTokenAmount; // @audit 100 ether

  // Keep track of total allocation auction tokens per epoch

  _totalAuctionTokenAllocation[auctionToken] = totalAuctionTokenAllocation + epochAuctionTokenAmount; // @audit 200 ether !!!!!

  We can see that the _totalAuctionTokenAllocation[auctionToken] is updated to 200 ether while it should have been updated to 100 ether because there is only ONE auction running with 100 ether tokens.

• Even if the DaoExecutor attempts to recover those extra 100 ether tokens stuck in the contract after the auction ends, he will not be able to do so. For simplicity, let's consider no one has claimed his tokens yet:

  function recoverToken(

  address token,

  address to,

  uint256 amount

  ) external override onlyDAOExecutor {

  // ...

  ​

  uint256 epochId = _currentEpochId;

  EpochInfo storage info = epochs[epochId];

  // ....

  uint256 totalAuctionTokenAllocation = _totalAuctionTokenAllocation[token]; // @audit 200 ether

  uint256 balance = IERC20(token).balanceOf(address(this)); // @audit 200 ether

  uint256 maxRecoverAmount = balance - (totalAuctionTokenAllocation - _claimedAuctionTokens[token]); // @audit = 0

  if (amount > maxRecoverAmount) { revert CommonEventsAndErrors.InvalidParam(); } // @audit tx will revert

  ​

  }

PoC

The following PoC demonstrates the example illustrated above. Copy/Paste the following code into test/forge/templegold/SpiceAuction.t.sol:

Impact

If the last auction config at cooldown is removed, two serious impacts will occur:

1. _totalAuctionTokenAllocation will always contain extra auction tokens for all subsequent epochs.

2. Those extra auction tokens can not be recovered.

Tools Used

Manual Review

Recommendations

Consider updating _totalAuctionTokenAllocation when removing the config of the last started auction at the cooldown period. Below is a suggestion for an updated code of SpiceAuction::removeAuctionConfig: