Summary
The purpose of an auction cooldown period is to allow changes to auctionConfig and aid token recovery before the auction bidding starts. This is prevented by a wrong implementation of auction active status check logic.
Vulnerability Details
An auction has three stages - cooldown period, active and ended. Updation of auctionConfig is not accepted when the auction is in the active stage but updates should be allowed during the cooldown period and when the auction has ended.
The following implementation checks if the auction hasEnded when it should check if the auction is isActive since hasEnded method checks whether info.endTime <= block.timestamp which also includes the cooldown period.
function setAuctionConfig(AuctionConfig calldata _config) external override onlyElevatedAccess {
if (_config.auctionStartCooldown == 0
|| _config.auctionMinimumDistributedGold == 0
|| _config.auctionsTimeDiff == 0)
{ revert CommonEventsAndErrors.ExpectedNonZero(); }
@> if (!epochs[_currentEpochId].hasEnded()) { revert InvalidOperation(); }
auctionConfig = _config;
emit AuctionConfigSet(_currentEpochId, _config);
}
L#69 will always revert as hasEnded will return false during the cooldown period.
Impact
This prevents the auctionConfig from being updated during the auction cooldown period.
Proof of Code
Add the following test into DaiGoldAuction.t.sol,
function test_ConfigUpdateRevertsDuringCooldown() public {
vm.startPrank(executor);
IDaiGoldAuction.AuctionConfig memory config = _getAuctionConfig();
daiGoldAuction.setAuctionConfig(config);
skip(1 days);
daiGoldAuction.startAuction();
vm.expectRevert(abi.encodeWithSelector(IAuctionBase.InvalidOperation.selector));
daiGoldAuction.setAuctionConfig(config);
vm.stopPrank();
}
Import the console package,
import { console} from "forge-std/console.sol";
Also update the vesting period params in DaiGoldAuction.t.sol::_configureTempleGold as follows,
function _configureTempleGold() private {
ITempleGold.DistributionParams memory params;
params.escrow = 60 ether;
params.gnosis = 10 ether;
params.staking = 30 ether;
templeGold.setDistributionParams(params);
ITempleGold.VestingFactor memory factor;
- factor.numerator = 2 ether;
+ factor.numerator = 1;
- factor.denominator = 1000 ether;
+ factor.denominator = 162 weeks; // 3 years
templeGold.setVestingFactor(factor);
templeGold.setStaking(address(goldStaking));
// whitelist
templeGold.authorizeContract(address(daiGoldAuction), true);
templeGold.authorizeContract(address(goldStaking), true);
templeGold.authorizeContract(teamGnosis, true);
}
Run forge test --mt test_ConfigUpdateRevertsDuringCooldown.
Recommendations
Update the DaiGoldAuction::setAuctionConfig() to revert if the auction isActive and allow for config changes during cooldown period or if it has ended,
function setAuctionConfig(AuctionConfig calldata _config) external override onlyElevatedAccess {
// code
- if (!epochs[_currentEpochId].hasEnded()) { revert InvalidOperation(); }
+ if (epochs[_currentEpochId].isActive()) { revert InvalidOperation(); }
// code
}
Test the above change against the following unit test by running forge test --mt test_MitigationSuccessful,
function test_MitigationSuccessful() public {
vm.startPrank(executor);
IDaiGoldAuction.AuctionConfig memory config = _getAuctionConfig();
daiGoldAuction.setAuctionConfig(config);
skip(1 days);
daiGoldAuction.startAuction();
uint256 currentEpochId = daiGoldAuction.currentEpoch();
vm.expectEmit(address(daiGoldAuction));
emit AuctionConfigSet(currentEpochId, config);
daiGoldAuction.setAuctionConfig(config);
skip(1 hours);
vm.expectRevert(abi.encodeWithSelector(IAuctionBase.InvalidOperation.selector));
daiGoldAuction.setAuctionConfig(config);
vm.stopPrank();
}
Tools Used
Manual Review and Foundry for POC.
