Skip to content

Latest commit

 

History

History
76 lines (58 loc) · 3.75 KB

M-04.md

File metadata and controls

76 lines (58 loc) · 3.75 KB
Raw

Config for next auction can be set while the current auction has not started yet

Summary

Spice auction's setAuctionConfig() can be called while the current auction is in the cooldown period, causing incorrect assumptions in the auction lifecycle.

Vulnerability Details

Spice auctions need to be configured before they can be started.

084:     function setAuctionConfig(SpiceAuctionConfig calldata _config) external onlyDAOExecutor {
085:         /// @dev epoch Id is only updated when auction starts. 
086:         /// @dev cannot set config for past or ongoing auction
087:         uint256 currentEpochIdCache = _currentEpochId;
088:         if (currentEpochIdCache > 0) {
089:             EpochInfo storage info = epochs[currentEpochIdCache];
090:             /// Cannot set config for ongoing auction
091:             if (info.isActive()) { revert InvalidConfigOperation(); }
092:         }
093:         if (_config.duration < MINIMUM_AUCTION_PERIOD 
094:             || _config.duration > MAXIMUM_AUCTION_DURATION
095:             || _config.waitPeriod > MAXIMUM_AUCTION_WAIT_PERIOD) { revert CommonEventsAndErrors.InvalidParam(); }
096:         /// @dev startCooldown can be zero
097:         if (_config.waitPeriod == 0
098:             || _config.minimumDistributedAuctionToken == 0) { revert CommonEventsAndErrors.ExpectedNonZero(); }
099:         if (_config.recipient == address(0)) { revert CommonEventsAndErrors.InvalidAddress(); }
100: 
101:         currentEpochIdCache += 1;
102:         auctionConfigs[currentEpochIdCache] = _config;
103:         emit AuctionConfigSet(currentEpochIdCache, _config);
104:     }

Lines 88-92 determine the lifecycle of the process by restricting when the next auction config can be defined. As the comment "Cannot set config for ongoing auction" indicates, configuration should be set when there is no started and ongoing auction.

However, the isActive() function only flags auctions that are between the start and end time.

09:     function isActive(IAuctionBase.EpochInfo storage info) internal view returns (bool) {
10:         return info.startTime <= block.timestamp && block.timestamp < info.endTime;
11:     }
12:

This condition does not include started auctions that are in the cooldown period. This means that setAuctionConfig() can still be called while the current auction has been started and is in the cooldown period.

Impact

Even though these are actions executed from the DAO, an incorrect usage could break the invariant and lead to false assumption. One of these can be seen clearly in the recoverToken() function as stated by the condition in line 252 and the associated comment in line 255:

252:         if (!info.hasEnded() && auctionConfigs[epochId+1].duration == 0) { revert RemoveAuctionConfig(); }
253:         
254: 
255:         /// @dev Now `auctionStart` is not triggered but `auctionConfig` is set (where _currentEpochId is not updated yet)

The logic above assumes that a configured auction (i.e. a duration different than zero) has not been started, as stated by "Now auctionStart is not triggered but auctionConfig is set", but this is false since auctions can be started and then setAuctionConfig() called in the cooldown period, as demonstrated above.

Tools Used

None.

Recommendations

Instead of using isActive(), change the condition to !info.hasEnded() which would correctly cover both cases of a started auction and a pending to be started auction.

    if (currentEpochIdCache > 0) {
        EpochInfo storage info = epochs[currentEpochIdCache];
        /// Cannot set config for ongoing auction
-       if (info.isActive()) { revert InvalidConfigOperation(); }
+       if (!info.hasEnded()) { revert InvalidConfigOperation(); }
    }