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

In `SpiceAuction::removeAuctionConfig` would revert and auction's cannot be removed

mrmorningstar

Summary

The issue is that auction config of auctions that started but their cooldown is not reached cannot be removed making the function not do of what it is indented to do.

Vulnerability Details

As we can see one of the intention for removeAuctionConfig function is to be able to remove and delete config for current auction that is started but the cooldown for auction is not reached.
https://github.com/Cyfrin/2024-07-templegold/blob/57a3e597e9199f9e9e0c26aab2123332eb19cc28/protocol/contracts/templegold/SpiceAuction.sol#L116

/// @dev could be that `auctionStart` is triggered but there's cooldown, which is not reached (so can delete epochInfo for _currentEpochId)

So the current way that it is done is this:
https://github.com/Cyfrin/2024-07-templegold/blob/57a3e597e9199f9e9e0c26aab2123332eb19cc28/protocol/contracts/templegold/SpiceAuction.sol#L119C1-L127C15

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 {

The current removeAuctionConfig check if auction in current epoch is active and if it has ended and revert if true as it should, but it does not check if auction is started but the cooldown was not reached so it can remove the current epoch and configuration as intended by protocol.
It should use EpochLib::hasStarted check to see if auction is started but the cooldown is not reached.
https://github.com/Cyfrin/2024-07-templegold/blob/57a3e597e9199f9e9e0c26aab2123332eb19cc28/protocol/contracts/templegold/EpochLib.sol#L17

The issue is also that also hasStarted has the wrong check.
https://github.com/Cyfrin/2024-07-templegold/blob/57a3e597e9199f9e9e0c26aab2123332eb19cc28/protocol/contracts/templegold/EpochLib.sol#L17

function hasStarted(IAuctionBase.EpochInfo storage info) internal view returns (bool) {
return info.startTime > 0 && block.timestamp >= info.startTime;
}

It will return true if startTime is bigger than 0 and if block.timestamp is equal or bigger than startTime.
If we check how startTime is calculated we can see this:
DaiGoldAuction - https://github.com/Cyfrin/2024-07-templegold/blob/57a3e597e9199f9e9e0c26aab2123332eb19cc28/protocol/contracts/templegold/DaiGoldAuction.sol#L122

uint128 startTime = info.startTime = uint128(block.timestamp) + config.auctionStartCooldown;

SpiceAuction - https://github.com/Cyfrin/2024-07-templegold/blob/57a3e597e9199f9e9e0c26aab2123332eb19cc28/protocol/contracts/templegold/SpiceAuction.sol#L169

uint128 startTime = info.startTime = uint128(block.timestamp) + config.startCooldown;

As we can see in both configuration's start time is sum of block.timestamp + starting coldown of the auction.

In isActive check se can see that auctions is considered active it block.timestamp is between start and end time:
https://github.com/Cyfrin/2024-07-templegold/blob/57a3e597e9199f9e9e0c26aab2123332eb19cc28/protocol/contracts/templegold/EpochLib.sol#L9

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

So in our hasStarted function it will return true if block.timestamp is bigger or equal of startTime which then it would go inisActive time frame making function hasStarted inaccurate and lose it's purpose (it's purpose is to show us that auction is started but cooldown is not reached)

Impact

Auction in SpiceAuction can't be removed as intended which will make protocol not be able to remove auctions and epoch that are started but the cooldown for starting auction's is not reached.

Tools Used

Manual Review.

Recommendations

First change the hasStarted function to correctly checks

function hasStarted(IAuctionBase.EpochInfo storage info) internal view returns (bool) {
- return info.startTime > 0 && block.timestamp >= info.startTime;
+ return info.startTime > 0 && block.timestamp <= info.startTime;
​
}

Then implement hasStarted check in removeAuctionConfig

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
+ if(info.hasStarted) {
delete auctionConfigs[id];
delete epochs[id];
_currentEpochId = id - 1;
emit AuctionConfigRemoved(id, id);
+ } { revert AuctionColdownIsReached();}
} else {

IAuctionBase
https://github.com/Cyfrin/2024-07-templegold/blob/57a3e597e9199f9e9e0c26aab2123332eb19cc28/protocol/contracts/interfaces/templegold/IAuctionBase.sol

error CannotDeposit();
error CannotClaim(uint256 epochId);
error CannotStartAuction();
error InvalidEpoch();
error AuctionActive();
error InvalidOperation();
error AuctionEnded();
+ error AuctionColdownIsReached();
Updates

Lead Judging Commences

inallhonesty Lead Judge over 1 year ago
Submission Judgement Published
Invalidated
Reason: Incorrect statement

Support

FAQs

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