TempleGold
TempleDAO
Foundry
25,000 USDC
View results
Previous Next
Submission Details
Severity: medium
Invalid

`SpiceAuction::setAuctionConfig` allows setting auction config during cooldown period, compromising protocol's auction integrity and predictability

kildren

Relevant GitHub Links

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

Summary

The protocol design intends for auctions to occur sequentially, limiting configuration operations to the two most recent epochs to ensure reliability and predictability of auction settings. Theoretically, new auction configurations should only be added after the previous auction has fully concluded.However, the current implementation of SpiceAuction::setAuctionConfig contains a vulnerability:

  1. It correctly restricts adding new auction configurations when an auction is active.

  2. It fails to prevent setting new configurations during the auction's cooldown period.

Vulnerability Details

function setAuctionConfig(SpiceAuctionConfig calldata _config) external onlyDAOExecutor {
/// @dev epoch Id is only updated when auction starts.
/// @dev cannot set config for past or ongoing auction
uint256 currentEpochIdCache = _currentEpochId;
if (currentEpochIdCache > 0) {
EpochInfo storage info = epochs[currentEpochIdCache];
/// Cannot set config for ongoing auction
@> if (info.isActive()) { revert InvalidConfigOperation(); }
}

This code only prevents setting new auction config when the current auction is in an active state. This check would erroneously pass when the auction is in a cooldown state. Logically, if setting a new config is not allowed in the active state, there is no reason for it to be allowed during the cooldown period.

POC

Place the following test into SpiceAuction.t.sol::SpiceAuctionTest. This test demonstrates how the flaw in SpiceAuction::setAuctionConfig affects the normal operation of SpiceAuction::removeAuctionConfig. The removeAuctionConfig function is expected to delete the most recent AuctionConfig that hasn't started or is in the cooldown period. Under normal circumstances, if the current Auction is in cooldown, we would expect it to remove this Auction. However, in this example, its behavior differs from the expected outcome.

function test_setSpiceAuctionConfigInCoolDown() public {
_startAuction(true, true);
// startCooldown is 1 hours
vm.warp(block.timestamp + 30 minutes);
​
// we can set new auctionConfig when current auction is in cooldown
ISpiceAuction.SpiceAuctionConfig memory config = _getAuctionConfig();
config.startCooldown = 2 hours;
vm.startPrank(daoExecutor);
// vm.expectRevert(abi.encodeWithSelector(ISpiceAuction.InvalidConfigOperation.selector));
spice.setAuctionConfig(config);
assertEq(spice.getAuctionConfig(0).startCooldown, 0);
assertEq(spice.getAuctionConfig(1).startCooldown, 1 hours);
assertEq(spice.getAuctionConfig(2).startCooldown, 2 hours);
​
// The config in cooldown is not removed as expected
spice.removeAuctionConfig();
assertEq(spice.getAuctionConfig(0).startCooldown, 0);
assertEq(spice.getAuctionConfig(1).startCooldown, 1 hours);
assertEq(spice.getAuctionConfig(2).startCooldown, 0);
}

Impact

This behavior may lead to the following issues:

  • Breaching the protocol's expected limitations on auction configuration operations.

  • Potentially altering rules after an auction has effectively begun, introducing unfairness.

  • Reducing system predictability, possibly causing confusion and distrust among participants.

The above case highlights a potential vulnerability in the contract's logic for managing auction configurations, which could lead to unexpected state changes and potential exploits.

Tools Used

Manual Review.

Recommendations

Replace info.isActive() with info.hasEnded() in the setAuctionConfig function. Ensure that new auction configurations can only be set when the current auction or the previous auction has completely ended. Prevent setting new configurations once startAuction() has been called for the current auction.

function setAuctionConfig(SpiceAuctionConfig calldata _config) external onlyDAOExecutor {
/// @dev epoch Id is only updated when auction starts.
/// @dev cannot set config for past or ongoing auction
uint256 currentEpochIdCache = _currentEpochId;
if (currentEpochIdCache > 0) {
EpochInfo storage info = epochs[currentEpochIdCache];
/// Cannot set config for ongoing auction
- if (info.isActive()) { revert InvalidConfigOperation(); }
+ if (!info.hasEnded()) { revert InvalidConfigOperation(); }
}
Updates

Lead Judging Commences

inallhonesty Lead Judge 11 months ago
Submission Judgement Published
Invalidated
Reason: Incorrect statement

Support

FAQs

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