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];
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:
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 {
uint256 id = _currentEpochId;
EpochInfo storage info = epochs[id];
if (info.startTime == 0) { revert InvalidConfigOperation(); }
if (info.isActive()) { revert InvalidConfigOperation(); }
bool configSetButAuctionStartNotCalled = auctionConfigs[id+1].duration > 0;
if (!configSetButAuctionStartNotCalled) {
if (info.hasEnded()) { revert AuctionEnded(); }
delete auctionConfigs[id];
delete epochs[id];
_currentEpochId = id - 1;
emit AuctionConfigRemoved(id, id);
} else {
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) {
if (info.hasEnded()) { revert AuctionEnded(); }
delete auctionConfigs[id];
delete epochs[id];
_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];
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:
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);
}