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

[info] `SpiceAuction::removeAuctionConfig` Fails for Initial AuctionConfig

kildren

Relevant GitHub Links

https://github.com/Cyfrin/2024-07-templegold/blob/main/protocol/contracts/templegold/SpiceAuction.sol#L113

Summary

When the first AuctionConfig is set but not started, using removeAuctionConfig will result in an error. Although it's possible to reset the AuctionConfig by calling setAuctionConfig again to overwrite the current AuctionConfig, this approach may confuse operators.

Vulnerability Details

When the auction hasn't started, the id in the function points to 0 (line 109). At this point, epochs[0].startTime == 0 (line 113) is always true, causing an error. This check seems intended to prevent the function from being called when no auction is set, but this scenario is already excluded at line 121. Therefore, this initial check is redundant and can be removed.

POC

Place the following test into SpiceAuction.t.sol::SpiceAuctionTest.

function test_removeAfterSetFirstAuctionConfigNotStart() public {
ISpiceAuction.SpiceAuctionConfig memory config = _getAuctionConfig();
vm.startPrank(daoExecutor);
spice.setAuctionConfig(config);
vm.expectRevert(abi.encodeWithSelector(ISpiceAuction.InvalidConfigOperation.selector));
spice.removeAuctionConfig();
}

Impact

For most calls, this check is ineffective and wastes gas. Furthermore, it makes the protocol's handling of removing the first AuctionConfig awkward. Removing this check would streamline the contract logic.

Tools Used

Manual review.

Recommendations

Either document this special case in the contract comments or remove the check for info.startTime:

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

This change will improve gas efficiency and make the contract's behavior more consistent when handling the first AuctionConfig.

Updates

Lead Judging Commences

inallhonesty Lead Judge 11 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.