Summary

removeAuctionConfig function in SpiceAuction.sol prevents DAOExecutor from removing the auction configuration for first epoch (i.e epoch id == 1). This issue restricts DAOExecutor from removing the setAuctionConfig for first epoch (if required) and forces startAuction in order to remove the configuration. However, this could escalate to a situation where if the startCooldown is set as zero, the auction starts immediately and the configuration can never be removed. This could be significant if the setAuctionConfig needs to be removed due to wrong set parameters that could cause harm for the protocol.

Vulnerability Details

The removeAuctionConfig function is designed to allow the DAOExecutor to remove the auction configuration for the current epoch or next epoch. To be precise, it only allows auctionConfig removal for current epoch if the auction for the current epoch is not active (and has not ended to prevent deleting old ended auctions) i.e in startCooldown period and auctionConfig removal for next epoch if auctionConfig for the next epoch is set.

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

While its not an ideal nature of DAOExecutor to provide wrong parameters, the existence of removeAuctionConfig function is to remove wrong auctionConfig parameters provided by mistake and also remove auctionConfig to cancel auctions in startCooldown period. However, the current implementation seems to have created a complication for the first epoch (i.e epoch id == 1) when the current epoch is zero (if (info.startTime == 0) { revert InvalidConfigOperation(); }), which restricts DAOExecutor from removing the setAuctionConfig for first epoch i.e if required.

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

This check is intended to ensure that the current epoch is valid (i.e cannot be zero since epoch 0 is a redundant epoch, info.startTime will be zero). However, when the current epoch is 0, it becomes impossible to remove the auction configuration for first epoch (i.e auctionConfig for epoch id 1 that has not started) without actually starting the auction. Therefore to remove auctionConfig for epoch id == 1 (if required) that has not started, startAuction will need to be triggered to start the auction before DAOExecutor will be able to remove auctionConfig for epoch id == 1 (since epoch id == 1 will be in startCooldown period once initiated). However, this can escalate badly if the startCooldown for epoch id == 1 is set to zero (note: startCooldown can be set as zero), which means the auction starts immediately, and auctionConfig for epoch id == 1 can never be removed, causing a permanent block in the removal process.

Therefore, if by chance the auctionConfig for epoch id == 1 needed to be removed due to wrong set parameters in auctionConfig e.g wrong recipient address, this will cause all bid tokens to be sent to a wrong recipient address which will cause a grave impact (note: impact will vary based on the reason why the auctionConfig needs to be removed). While its unexpected of the DAOExecutor to provide wrong parameters for auctionConfig, the existence of removeAuctionConfig function suggests the possibility and the need to cancel an auction.

Impact

First epoch (i.e epoch 1) cannot be removed if required which could cause variable severe impacts (e.g loss of bid tokens to wrong recipient address).

POC

Run command forge test --match-test test_removeAuctionConfig_POC in SpiceAuction.t.sol

Tools Used

Manual

Recommendations

Remove this check below as its very unlikely to remove auctionConfig for epoch id == 0 because epoch 0 is redundant. Therefore, if current epoch = 0, and DAOExecutor want to remove auctionConfig for epoch id == 1, removeAuctionConfig will remove auctionConfig for epoch id == 1 (the next epoch) if set.

Please note this issue also exists in the recover function therefore will need to be fixed on both ends to fully mitigate this issue else auction tokens for the first epoch will be irrevocable https://github.com/Cyfrin/2024-07-templegold/blob/57a3e597e9199f9e9e0c26aab2123332eb19cc28/protocol/contracts/templegold/SpiceAuction.sol#L251