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

Config for the first ever spice auction cannot be removed before auction start

Trident Audits

Summary

The SpiceAuction::removeAuctionConfig() function doesn't allow the config for the first ever spice auction (auction id 1) to be removed unless the auction is in cooldown period. If the cooldown period is 0, then the auction config can never be removed.

NOTE
Auction id isn't a state variable. It is used to better represent the latest auction number. Auction config is set for the _currentEpochId + 1 index, and the _currentEpochId variable lags behind the auction id by 1 until the auction starts. Then the _currentEpochId is incremented, and epoch info is set.

Vulnerability Details

The SpiceAuction::removeAuctionConfig() function can be used to remove the config set for an auction before it starts or when it's in cooldown period. This works correctly for all auctions with id greater than 1, however, for auction id 1, which has not started yet (epoch info hasn't been set, with epoch id at 0 and auction id at 1), the function will always revert.

Consider the following code segment from the SpiceAuction contract,

/// @notice Remove auction config set for last epoch
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();
​
// ...
​
}

Suppose, auction config has been set using the SpiceAuction::setAuctionConfig() function with id 1. However, epoch id isn't incremented and epoch info isn't set until SpiceAuction::startAucion() function is called. So the SpiceAuction::removeAuctionConfig() function reads a non-existent epoch info (with id 0). All fields in the EpochInfo struct hold value 0. This will cause a revert at L#113.

It is to be noted that auction config can be removed once the auction starts and is in cooldown period. However, what makes this issue severe is that spice auctions can have no cooldown period. This can be seen in the following code segment from SpiceAuction::setAuctionConfig(),

function setAuctionConfig(SpiceAuctionConfig calldata _config) external onlyDAOExecutor {
// ...
​
if (
_config.duration < MINIMUM_AUCTION_PERIOD || _config.duration > MAXIMUM_AUCTION_DURATION
|| _config.waitPeriod > MAXIMUM_AUCTION_WAIT_PERIOD
) revert CommonEventsAndErrors.InvalidParam();
@> /// @dev startCooldown can be zero
if (_config.waitPeriod == 0 || _config.minimumDistributedAuctionToken == 0) {
revert CommonEventsAndErrors.ExpectedNonZero();
}
​
// ...
}

There is no check to see if the starting cooldown period is greater than 0.

Impact

In case where the first spice auction needs to be taken down, the dao executer won't be able to do so unless the auction has started and is in cooldown period. However, if the auction config for the first spice auction has no cooldown period, then the auction can never be removed.

Proof of Concept

Add the following test function to the SpiceAuctionTest contract in ./protocol/test/forge/templegold/SpiceAuction.t.sol,

function testRemovingAuctionConfigBeforeAuctionStartForFirstAuctionFails() public {
ISpiceAuction.SpiceAuctionConfig memory config;
config = _getAuctionConfig();
vm.startPrank(daoExecutor);
spice.setAuctionConfig(config);
vm.stopPrank();
​
address auctionToken = config.isTempleGoldAuctionToken ? address(templeGold) : spice.spiceToken();
dealAdditional(IERC20(auctionToken), address(spice), 100 ether);
​
vm.expectRevert(ISpiceAuction.InvalidConfigOperation.selector);
vm.prank(daoExecutor);
spice.removeAuctionConfig();
}

The test runs successfully, with the following logs,

Ran 1 test for test/SpiceAuction.t.sol:SpiceAuctionTest
[PASS] testRemovingAuctionConfigBeforeAuctionStartForFirstAuctionFails() (gas: 395916)
Suite result: ok. 1 passed; 0 failed; 0 skipped; finished in 914.28ms (2.64ms CPU time)
​
Ran 1 test suite in 936.04ms (914.28ms CPU time): 1 tests passed, 0 failed, 0 skipped (1 total tests)

Tools Used

Manual review, and Foundry for writing POC and tests.

Recommended Mitigation

Make the following changes to SpiceAuction::removeAuctionConfig() function,

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();
+ if (id != 0) {
+ // _currentEpochId = 0
+ if (info.startTime == 0) revert InvalidConfigOperation();
+ // Cannot reset an ongoing auction
+ if (info.isActive()) revert InvalidConfigOperation();
+ }
​
// ...
}

Now let's unit test the changes. Add the following test to the SpiceAuctionTest contract in ./protocol/test/forge/templegold/SpiceAuction.t.sol,

function testRemoveAuctionConfigBeforeAuctionStartForFirstAuction() public {
ISpiceAuction.SpiceAuctionConfig memory config;
config = _getAuctionConfig();
vm.startPrank(daoExecutor);
spice.setAuctionConfig(config);
vm.stopPrank();
​
address auctionToken = config.isTempleGoldAuctionToken ? address(templeGold) : spice.spiceToken();
dealAdditional(IERC20(auctionToken), address(spice), 100 ether);
​
vm.prank(daoExecutor);
spice.removeAuctionConfig();
}

The test passes with the following logs,

Ran 1 test for test/SpiceAuction.t.sol:SpiceAuctionTest
[PASS] testRemoveAuctionConfigBeforeAuctionStartForFirstAuction() (gas: 395766)
Suite result: ok. 1 passed; 0 failed; 0 skipped; finished in 1.05s (2.97ms CPU time)
​
Ran 1 test suite in 1.07s (1.05s CPU time): 1 tests passed, 0 failed, 0 skipped (1 total tests)
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.