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

Incomplete Auction Epoch Management Allows Bypassing `prevAuctionInfo.endTime` in DaiAuction Contract

recursiveeth

Summary

The recoverToken function in your Solidity contract deletes the current auction epoch without adjusting the currentEpochId. This can lead to inconsistencies and unintended behavior, particularly affecting the startAuction function, which relies on the correct management of epochs. This oversight allows bypassing the previous auction data which will be zero, potentially disrupting the auction process and protocol functionality.

Vulnerability Details

as DAI auction is simple don't have much complexity as spice, so i will go with the flow owner will call setAuctionconfig then startAuction and suppose i have already performed set of auctions and now the currentEpochId = 10, as daiAuction contract don't want us to set cool down period as 0, so suppose i have called startAuction 11th time and after it executes Current epochid == 11, now suppose i want to cancel this auction so i will call recover tokens function which checks whether the auction is started or ended basically to clarify whether its in cool down period or not after that if it didn't revert it will delete epoch , but one they forgot is to subtract the epoch id from total, the issue occur when the start Auction wll be called and currentEpochid will be 11 so the if statment which check the preAuction.endtime which will be 0, so block.timestamp should be just need to greater than config.auctionsTimeDiff, and the epoch Data will behave like auction is called for first time which create issue while making new auctions

Incomplete Epoch Deletion: The recoverToken function deletes the current epoch (delete epochs[epochId];) but does not adjust the currentEpochId. This leaves the currentEpochId in an inconsistent state, which can lead to issues in subsequent function calls that rely on epoch management.

Incorrect Epoch State: With the incorrect handling of currentEpochId, the contract logic can enter a state where auctions are started without respecting the intended cooldown period. This result in unintended auction behaviors.

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

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

//START AUCTIONS
function startAuction() external override {
if (auctionStarter != address(0) && msg.sender != auctionStarter) { revert CommonEventsAndErrors.InvalidAccess(); }
EpochInfo storage prevAuctionInfo = epochs[_currentEpochId];
if (!prevAuctionInfo.hasEnded()) { revert CannotStartAuction(); }
AuctionConfig storage config = auctionConfig;
/// @notice last auction end time plus wait period
if (_currentEpochId > 0 && (prevAuctionInfo.endTime + config.auctionsTimeDiff > block.timestamp)) {
revert CannotStartAuction();
}
_distributeGold();
uint256 totalGoldAmount = nextAuctionGoldAmount;
nextAuctionGoldAmount = 0;
uint256 epochId = _currentEpochId = _currentEpochId + 1;
if (totalGoldAmount < config.auctionMinimumDistributedGold) { revert LowGoldDistributed(totalGoldAmount); }
​
EpochInfo storage info = epochs[epochId];
info.totalAuctionTokenAmount = totalGoldAmount;
uint128 startTime = info.startTime = uint128(block.timestamp) + config.auctionStartCooldown;
uint128 endTime = info.endTime = startTime + AUCTION_DURATION;
​
emit AuctionStarted(epochId, msg.sender, startTime, endTime, totalGoldAmount);
}
//RECOVER TOKENS
function recoverToken(
address token,
address to,
uint256 amount
) external override onlyElevatedAccess {
if (to == address(0)) { revert CommonEventsAndErrors.InvalidAddress(); }
if (amount == 0) { revert CommonEventsAndErrors.ExpectedNonZero(); }
​
if (token != address(templeGold)) {
emit CommonEventsAndErrors.TokenRecovered(to, token, amount);
IERC20(token).safeTransfer(to, amount);
return;
}
​
// auction started but cooldown pending
uint256 epochId = _currentEpochId;
EpochInfo storage info = epochs[epochId];
if (info.startTime == 0) { revert InvalidOperation(); }
if (info.isActive()) { revert AuctionActive(); }
if (info.hasEnded()) { revert AuctionEnded(); }
uint256 _totalAuctionTokenAmount = info.totalAuctionTokenAmount;
if (amount > _totalAuctionTokenAmount) { revert CommonEventsAndErrors.InvalidAmount(token, amount); }
/// @dev Epoch data is deleted and leftover amount is addedd to nextAuctionGoldAmount.
/// so admin should recover total auction amount for epoch if that's the requirement
delete epochs[epochId];
/// @dev nextAuctionGoldAmount is set to 0 in startAuction.
/// nextAuctionGoldAmount > 0 if there was a distribution after auctionStart called
/// epoch is deleted. so if amount < totalAuctionTokenAmount for epoch, add leftover to next auction amount
unchecked {
nextAuctionGoldAmount += _totalAuctionTokenAmount - amount;
}
​
emit CommonEventsAndErrors.TokenRecovered(to, token, amount);
templeGold.safeTransfer(to, amount);
}
​

Impact

The impact of this vulnerability is significant and includes:

  • Protocol Disruption: The ability to bypass the cooldown period can disrupt the normal operation of creating new Auction and other checks performed while creating New auction and intended pacing of auctions, leading to potential protocol instability or abuse.

  • Inconsistent State: The contract may enter an inconsistent state where the currentEpochId does not accurately reflect the actual state of auctions, leading to further logical errors or vulnerabilities.

Tools Used

Manual Review

Recommendations

To mitigate this issue, consider implementing the deletion operation *in *removetoken function

Adjust currentEpochId on Deletion: Ensure that when an epoch is deleted via recoverToken, the currentEpochId is appropriately adjusted to maintain consistency. This could involve decrementing the currentEpochId or implementing a more robust epoch management system.

//RECOVER TOKENS
function recoverToken(address token,
address to,
uint256 amount
) external override onlyElevatedAccess {
if (to == address(0)) { revert CommonEventsAndErrors.InvalidAddress(); }
if (amount == 0) { revert CommonEventsAndErrors.ExpectedNonZero(); }
​
if (token != address(templeGold)) {
emit CommonEventsAndErrors.TokenRecovered(to, token, amount);
IERC20(token).safeTransfer(to, amount);
return;
}
​
// auction started but cooldown pending
uint256 epochId = _currentEpochId;
EpochInfo storage info = epochs[epochId];
if (info.startTime == 0) { revert InvalidOperation(); }
if (info.isActive()) { revert AuctionActive(); }
if (info.hasEnded()) { revert AuctionEnded(); }
uint256 _totalAuctionTokenAmount = info.totalAuctionTokenAmount;
if (amount > _totalAuctionTokenAmount) { revert CommonEventsAndErrors.InvalidAmount(token, amount); }
/// @dev Epoch data is deleted and leftover amount is addedd to nextAuctionGoldAmount.
/// so admin should recover total auction amount for epoch if that's the requirement
delete epochs[epochId];
/// @dev nextAuctionGoldAmount is set to 0 in startAuction.
/// nextAuctionGoldAmount > 0 if there was a distribution after auctionStart called
/// epoch is deleted. so if amount < totalAuctionTokenAmount for epoch, add leftover to next auction amount
+ _currentEpochId = _currentEpochId - 1;
​
unchecked {
nextAuctionGoldAmount += _totalAuctionTokenAmount - amount;
}
​
emit CommonEventsAndErrors.TokenRecovered(to, token, amount);
templeGold.safeTransfer(to, amount);
}
Updates

Lead Judging Commences

inallhonesty Lead Judge 12 months ago
Submission Judgement Published
Invalidated
Reason: Incorrect statement
Assigned finding tags:

two auctions can be started with less than auctionsTimeDiff time difference due to recoverToken deleting the epochs[epochId]

Support

FAQs

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