Summary
When an auction ends with 0 bids, the totalAuctionTokenAmount value of TGLD tokens remain locked forever as DaiGoldAuction::recoverToken() function can only be used when the auction is still in its cooldown period.
Vulnerability Details
DaiGoldAuction::recoverToken provides a mechanism to recover tokens that have been distributed and allotted to a specific auction only BEFORE the auction starts, that is during the cooldown period as evident by the code below,
function recoverToken(
address token,
address to,
uint256 amount
) external override onlyElevatedAccess {
uint256 epochId = _currentEpochId;
EpochInfo storage info = epochs[epochId];
if (info.startTime == 0) { revert InvalidOperation(); }
@> if (info.isActive()) { revert AuctionActive(); }
@> if (info.hasEnded()) { revert AuctionEnded(); }
}
When users bid in an auction, they are entitled to their claims, even if there's just one bid, that bidder deserves the entire pot. Users are also allowed to claim tokens from a previous auction as they please, hence we need not worry about tokens remaining stuck in the contract.
However this is not the case when an auction ends with no bids, as in such a situation we would want to recover those tokens as there are no claims. This is prevented by the fact that tokens cannot be recovered after an auction ends.
Impact
totalAuctionTokenAmount value worth of tokens allotted for the auction would remain permanently stuck inside the contract. If this scenario repeats multiple times, a significant amount of tokens would be cut out from circulation.
Proof of Code
Add the following test to the existing DaiGoldAuction.t.sol file,
function test_CannotRecover_AuctionTokens_WhenZeroBids() public{
IDaiGoldAuction.AuctionConfig memory config = _getAuctionConfig();
vm.startPrank(executor);
daiGoldAuction.setAuctionConfig(config);
skip(1 days);
daiGoldAuction.startAuction();
THE AUCTION ENDS WITHOUT ANY BIDS
*/
uint256 currentEpoch = daiGoldAuction.currentEpoch();
IAuctionBase.EpochInfo memory info1 = daiGoldAuction.getEpochInfo(currentEpoch);
vm.warp(info1.endTime);
uint256 recoveryAmount1 = info1.totalAuctionTokenAmount;
vm.expectRevert(abi.encodeWithSelector(IAuctionBase.AuctionEnded.selector));
daiGoldAuction.recoverToken(address(templeGold), alice, recoveryAmount1);
assertEq(daiGoldAuction.nextAuctionGoldAmount(), 0);
vm.warp(info1.endTime + config.auctionsTimeDiff);
daiGoldAuction.startAuction();
currentEpoch = daiGoldAuction.currentEpoch();
IAuctionBase.EpochInfo memory info2 = daiGoldAuction.getEpochInfo(currentEpoch);
uint256 recoveryAmount2 = info2.totalAuctionTokenAmount;
uint256 totalRecoveryAmount = recoveryAmount1 + recoveryAmount2;
vm.expectRevert(abi.encodeWithSelector(CommonEventsAndErrors.InvalidAmount.selector, address(templeGold), totalRecoveryAmount));
daiGoldAuction.recoverToken(address(templeGold), alice, totalRecoveryAmount);
assertEq(templeGold.balanceOf(address(daiGoldAuction)), totalRecoveryAmount);
}
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_CannotRecover_AuctionTokens_WhenZeroBids -vv
The test passes successfully with the following logs,
Ran 1 test for test/forge/templegold/DaiGoldAuction.t.sol:DaiGoldAuctionTest
[PASS] test_CannotRecover_AuctionTokens_WhenZeroBids() (gas: 405414)
Suite result: ok. 1 passed; 0 failed; 0 skipped; finished in 475.59ms (2.81ms CPU time)
Ran 1 test suite in 483.13ms (475.59ms CPU time): 1 tests passed, 0 failed, 0 skipped (1 total tests)
Recommendations
Preferable mitigation would be to add another function to DaiGoldAuction for specifically recovering tokens when an auction ends with 0 bids as shown below:
+ /**
+ * @notice Recover auction tokens for epoch with zero bids
+ * @param epochId Epoch Id
+ * @param to Recipient
+ */
+ function recoverAuctionTokenForZeroBidAuction(uint256 epochId, address to) external onlyElevatedAccess {
+ if (to == address(0)) { revert CommonEventsAndErrors.InvalidAddress(); }
+ // has to be valid epoch
+ if (epochId > _currentEpochId) { revert InvalidEpoch(); }
+ // epoch has to be ended
+ EpochInfo storage info = epochs[epochId];
+ if (!info.hasEnded()) { revert AuctionActive(); }
+ // bid token amount for epoch has to be 0
+ if (info.totalBidTokenAmount > 0) { revert InvalidOperation(); }
+ uint256 amount = info.totalAuctionTokenAmount;
+ emit CommonEventsAndErrors.TokenRecovered(to, address(templeGold), amount);
+ templeGold.safeTransfer(to, amount);
+ }
Above change can be verified against the following uint test,
function test_MitigationSuccessful() public {
IDaiGoldAuction.AuctionConfig memory config = _getAuctionConfig();
vm.startPrank(executor);
daiGoldAuction.setAuctionConfig(config);
skip(1 days);
daiGoldAuction.startAuction();
THE AUCTION ENDS WITHOUT ANY BIDS
*/
uint256 currentEpoch = daiGoldAuction.currentEpoch();
IAuctionBase.EpochInfo memory info = daiGoldAuction.getEpochInfo(currentEpoch);
vm.warp(info.endTime);
uint256 recoveryAmount = info.totalAuctionTokenAmount;
vm.expectEmit(address(daiGoldAuction));
emit TokenRecovered(alice, address(templeGold), recoveryAmount);
daiGoldAuction.recoverAuctionTokenForZeroBidAuction(currentEpoch, alice);
assertEq(templeGold.balanceOf(alice), recoveryAmount);
}
The test passes successfully with the following logs,
Ran 1 test for test/forge/templegold/DaiGoldAuction.t.sol:DaiGoldAuctionTest
[PASS] test_MitigationSuccessful() (gas: 329745)
Suite result: ok. 1 passed; 0 failed; 0 skipped; finished in 455.58ms (3.28ms CPU time)
Ran 1 test suite in 458.30ms (455.58ms CPU time): 1 tests passed, 0 failed, 0 skipped (1 total tests)
Another way would be to update recoverToken to also account for 0 bid auctions but it is better to have another function for the sake of Separation of Concerns.
Tools Used
Manual Review and Foundry for POC.
