TempleGold

Summary

auctionsTimeDiff for DAIGoldAuctions can be bypassed in a special edge-case whereby an authorized admin recovers auction tokens for an auction in cooldown period subsequently because the recoverToken function on DAIGoldAuction.sol deletes epochs[epochId] (i.e EpochInfo) but does not decrement the _currentEpochId.

Vulnerability Details

recoverToken function on DAIGoldAuction.sol allows an authorized admin to recover a specified token amount to a given address. The function differentiates between an arbitrary token and templeGold token, with additional logic for handling the recovery of templeGold token.
For templeGold token recovery, recoverToken function checks to ensure that current epoch is valid and not currently active or already ended (i.e in auctionStartCooldown period) while also ensuring that the specified amount to be recovered does not exceed the total supply auction amount of the auction. The EpochInfo is deleted while the specified amount is sent to the given address and the leftover amount is added to nextAuctionGoldAmount.
https://github.com/Cyfrin/2024-07-templegold/blob/57a3e597e9199f9e9e0c26aab2123332eb19cc28/protocol/contracts/templegold/DaiGoldAuction.sol#L260-L294

However, _currentEpochId is not decremented after the auction tokens for that epoch have been recovered (either sent out or stored for the next auction) and deleted the EpochInfo. This allows auctionsTimeDiff to be bypassed for the next epoch to start an auction for because info.endTime for the _currentEpochId will no longer exist (i.e info.endTime = 0) which is a requirement for the auctionsTimeDiff check before starting a new auction.

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

This means the next epoch to start an auction for can readily be initiated to start by anyone (i.e if auctionStarter is not set and the recovered auction amount is stored in nextAuctionGoldAmount for the next epoch) right after an auction has been cancelled or recovered by admins. This will be an issue especially if the admins decide to cancel and recover auction tokens for _currentEpochId (and store in nextAuctionGoldAmount for the next epoch) and want to extend the next auction initialization or epoch.

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

Although this issue can be avoided if a trusted auctionStarter is set (but auctionStarter will likely not always be set) and admins will spot and cancel the initiated auction again (and recover the auction tokens) i.e if the auctionStartCooldown has not passed (which falls the severity of this issue to - LOW).

Impact

auctionsTimeDiff can be bypassed.

POC

• Assume Admins want to cancel an auction in auctionStartCooldown period and recover the auction tokens to extend starting next epoch or auction.

• Admins recover templeGold tokens for the auction (assume _currentEpochId = 2 and auctionsTimeDiff = 2 weeks i.e. extend for 2 weeks).

• The auction tokens are recovered and an adequate amount is stored for the next auction (i.e. stored in nextAuctionGoldAmount).

• Alice calls startAuction() (assuming auctionStarter is not set since admins just want to extend the next epoch starting).

• However _currentEpochId is still = 2 therefore this check if (_currentEpochId > 0 && (prevAuctionInfo.endTime + config.auctionsTimeDiff > block.timestamp)) { revert CannotStartAuction(); } is bypassed.

• This is because prevAuctionInfo.endTime for _currentEpochId = 2 no longer exist i.e prevAuctionInfo.endTime = 0. Therefore (prevAuctionInfo.endTime(0) + config.auctionsTimeDiff(2 weeks)) is not greater than block.timestamp which successfully bypasses the check and starts the nex epoch and auction.

• In the best case scenario admins will spot and cancel the initiated auction again (and recover the auction tokens) i.e if the auctionStartCooldown has not passed while in the worst case scenario if the admins fail to spot and the auctionStartCooldown pass, the auction startTime begins therefore cannot be cancelled or no longer recoverable.

Tools Used

Manual

Recommendations

• Decrement _currentEpochId in recoverToken function (although this does not completely mitigate the issue as admins will have to increase auctionsTimeDiff to extend the next epoch).

• set endtime for the _currentEpochId after deleting the EpochInfo (this seems to be a better fix).