TempleGold
TempleDAO
Foundry
25,000 USDC
View results
Previous Next
Submission Details
Severity: low
Valid

Auction tokens that were mistakenly sent to the Spice contract in the first auction epoch can not be recovered

0xbrivan2

Vulnerability Details

In SpiceAuction, recoverToken function is meant to be used to recover auction tokens that were mistakenly sent to the contract for last but not started auction:

function recoverToken(
address token,
address to,
uint256 amount
) external override onlyDAOExecutor {
if (to == address(0)) { revert CommonEventsAndErrors.InvalidAddress(); }
if (amount == 0) { revert CommonEventsAndErrors.ExpectedNonZero(); }
​
if (token != spiceToken && token != templeGold) {
emit CommonEventsAndErrors.TokenRecovered(to, token, amount);
IERC20(token).safeTransfer(to, amount);
return;
}
uint256 epochId = _currentEpochId;
EpochInfo storage info = epochs[epochId];
/// @dev use `removeAuctionConfig` for case where `auctionStart` is called and cooldown is still pending
if (info.startTime == 0) { revert InvalidConfigOperation(); }
if (!info.hasEnded() && auctionConfigs[epochId+1].duration == 0) { revert RemoveAuctionConfig(); } // <------------------------------------
// ...
IERC20(token).safeTransfer(to, amount);
}

From the code snippet, more particularly in this check:

// @audit if `auctionConfigs[epochId+1].duration == 0`, it means we are the latest, unfinished auction epoch
if (!info.hasEnded() && auctionConfigs[epochId+1].duration == 0) { revert RemoveAuctionConfig(); }

we can see that if we are recovering auction tokens and the last auction epoch is at cooldown, removeAuctionConfig should be used first to remove the configuration of the last epoch.

Below is the code of removeAuctionConfig:

function removeAuctionConfig() external override onlyDAOExecutor {
/// only delete latest epoch if auction is not started
uint256 id = _currentEpochId;
EpochInfo storage info = epochs[id];
// _currentEpochId = 0
if (info.startTime == 0) { revert InvalidConfigOperation(); }
// Cannot reset an ongoing auction
if (info.isActive()) { revert InvalidConfigOperation(); }
/// @dev could be that `auctionStart` is triggered but there's cooldown, which is not reached (so can delete epochInfo for _currentEpochId)
// or `auctionStart` is not triggered but `auctionConfig` is set (where _currentEpochId is not updated yet)
bool configSetButAuctionStartNotCalled = auctionConfigs[id+1].duration > 0;
if (!configSetButAuctionStartNotCalled) {
/// @dev unlikely because this is a DAO execution, but avoid deleting old ended auctions
if (info.hasEnded()) { revert AuctionEnded(); }
/// auction was started but cooldown has not passed yet
delete auctionConfigs[id];
delete epochs[id];
_currentEpochId = id - 1;
emit AuctionConfigRemoved(id, id);
} else {
// `auctionStart` is not triggered but `auctionConfig` is set
id += 1;
delete auctionConfigs[id];
emit AuctionConfigRemoved(id, 0);
}
}

Because we are removing the config of the latest auction at cooldown, the following part of code will be executed:

if (!configSetButAuctionStartNotCalled) {
/// @dev unlikely because this is a DAO execution, but avoid deleting old ended auctions
if (info.hasEnded()) { revert AuctionEnded(); }
/// auction was started but cooldown has not passed yet
delete auctionConfigs[id];
delete epochs[id]; // @audit Notice that we delete the epoch
_currentEpochId = id - 1;
emit AuctionConfigRemoved(id, id);
}

We can see that the function deletes the epoch and decrements the epochId.

The issue is that the recoverToken function does not handle properly the recover process for the first auction, i.e when there is only auction. Consider the following example:

  • We have one started epoch with 100 ether auction tokens, it is at cooldown.

  • 2 ether were mistakenly sent to the contract during cooldown. The DaoExecutor attempts to recover them by calling recoverToken

  • Given that we are at the latest auction at cooldown, the config must be removed first:

    if (!info.hasEnded() && auctionConfigs[epochId+1].duration == 0) { revert RemoveAuctionConfig(); }

    So, the DaoExecutor calls removeAuctionConfig. Notice that after the tx, epochId will be decremented to 0.

  • Now, if we attempt to recover back the auction tokens, the transaction will still revert, but this time because of invalid epoch:

    EpochInfo storage info = epochs[epochId];
    /// @dev use `removeAuctionConfig` for case where `auctionStart` is called and cooldown is still pending
    if (info.startTime == 0) { revert InvalidConfigOperation(); }

  • If the new epoch (with id 1) is started, that 2 ether will be counted for the epoch auction tokens amount. And thus, can not be recovered:

    // function: startAuction
    ​
    uint256 balance = IERC20(auctionToken).balanceOf(address(this));
    uint256 epochAuctionTokenAmount = balance - (totalAuctionTokenAllocation - _claimedAuctionTokens[auctionToken]);

As a consequence, tokens mistakenly sent to the first epoch during the cooldown period can not be recovered.

Impact

Auction tokens that were mistakenly sent to the first epoch during the cooldown period can not be recovered.

Tools Used

Manual Review

Recommendations

Consider handling the case of recovering auction tokens for the first epoch. Below is a suggestion for an updated code of recoverToken:

function recoverToken(
address token,
address to,
uint256 amount
) external override onlyDAOExecutor {
if (to == address(0)) { revert CommonEventsAndErrors.InvalidAddress(); }
if (amount == 0) { revert CommonEventsAndErrors.ExpectedNonZero(); }
​
if (token != spiceToken && token != templeGold) {
emit CommonEventsAndErrors.TokenRecovered(to, token, amount);
IERC20(token).safeTransfer(to, amount);
return;
}
​
uint256 epochId = _currentEpochId;
EpochInfo storage info = epochs[epochId];
/// @dev use `removeAuctionConfig` for case where `auctionStart` is called and cooldown is still pending
if (info.startTime == 0) { revert InvalidConfigOperation(); }
+ if (!info.isActive() && !info.hasEnded() && epochId == 1) {
+ uint256 balance = IERC20(token).balanceOf(address(this));
+ uint256 recoverAmount = balance - totalAuctionTokenAllocation;
+ IERC20(token).safeTransfer(to, recoverAmount);
+ emit CommonEventsAndErrors.TokenRecovered(to, token, recoverAmount);
+ return;
+ }
if (!info.hasEnded() && auctionConfigs[epochId+1].duration == 0) { revert RemoveAuctionConfig(); } // audit-finding Contradiction?
​
/// @dev Now `auctionStart` is not triggered but `auctionConfig` is set (where _currentEpochId is not updated yet)
// check to not take away intended tokens for claims
// calculate auction token amount
uint256 totalAuctionTokenAllocation = _totalAuctionTokenAllocation[token];
uint256 balance = IERC20(token).balanceOf(address(this));
uint256 maxRecoverAmount = balance - (totalAuctionTokenAllocation - _claimedAuctionTokens[token]);
if (amount > maxRecoverAmount) { revert CommonEventsAndErrors.InvalidParam(); }
IERC20(token).safeTransfer(to, amount);
​
emit CommonEventsAndErrors.TokenRecovered(to, token, amount);
}
Updates

Lead Judging Commences

inallhonesty Lead Judge 12 months ago
Submission Judgement Published
Validated
Assigned finding tags:

`removeAuctionConfig` can't remove the first added `SpiceAuctionConfig` which in the end leads to inability to recover the funds associated to that auction

Support

FAQs

Can't find an answer? Chat with us on Discord, Twitter or Linkedin.