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

Restrict `to` Address in `DaiGoldAuction::recoverToken` to Prevent Unauthorized Transfer of templeGold

kildren

Relevant GitHub Links

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

Summary

The minted templeGold is strictly restricted to be used in stake, escrow, and team gnosis contracts. However, the recoverToken function in the DaiGoldAuction.sol escrow contract allows privileged users to transfer auction tokens directly, bypassing the auction process. This function is intended as a remedy for misconfigured auction settings, but it poses a risk to the protocol's funds and raises concerns about the protocol's security among users.

Vulnerability Details

At Line 260, the protocol allows privileged users to transfer templeGold intended for auction directly, without restrictions on the destination address. Given the relatively lenient access control over this function, there is a risk of granting permissions to inappropriate parties, potentially leading to fund misappropriation.

function recoverToken(
address token,
address to,
uint256 amount
@> ) external override onlyElevatedAccess {
...
​
// 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);
}

POC

Place the following test into DaiGoldAuction.t.sol::DaiGoldAuctionTest:

function test_recoverTokenToAttacker() public {
_setVestingFactor(templeGold);
skip(1 days);
vm.startPrank(executor);
templeGold.mint();
uint256 mintAmount = templeGold.balanceOf(address(daiGoldAuction));
​
IDaiGoldAuction.AuctionConfig memory _config = _getAuctionConfig();
daiGoldAuction.setAuctionConfig(_config);
_startAuction();
IAuctionBase.EpochInfo memory info = daiGoldAuction.getEpochInfo(1);
vm.warp(info.startTime - 10 seconds);
​
address attacker = makeAddr("attack");
uint256 _beforeRecoverBalance = templeGold.balanceOf(attacker);
assertEq(_beforeRecoverBalance, 0);
vm.startPrank(executor);
daiGoldAuction.recoverToken(address(templeGold), attacker, mintAmount);
uint256 _afterRecoverBalance = templeGold.balanceOf(attacker);
assertEq(_afterRecoverBalance, mintAmount);
}

Impact

  • For the project team, there's a risk of fund loss if recovery permissions are granted to the wrong individuals.

  • For users, the project team has the ability to bypass the strict issuance constraints of templeGold, making its value unpredictable and casting doubt on the long-term value of the project, which could reduce user engagement.

Tools Used

Manual Review.

Recommendations

  1. Impose Stricter Restrictions on the Method: Ensure that this function can only be used under specific conditions such as during rescueMode or by a designated executor.

  2. Restrict to Address when Handling templeGold: Validate the to address to ensure it is a multisignature wallet, thus limiting the flow of these templeGold tokens to trusted destinations.

function recoverToken(
address token,
address to,
uint256 amount
) external override onlyElevatedAccess {
...
+ // Add checks to ensure 'to' address is among trusted addresses
+ require(isTrustedAddress(to), "Invalid recovery address");
// auction started but cooldown pending
uint256 epochId = _currentEpochId;
​
...
​
emit CommonEventsAndErrors.TokenRecovered(to, token, amount);
templeGold.safeTransfer(to, amount);
}
​
+ // Example function to check trusted addresses
+ function isTrustedAddress(address _address) internal view returns (bool) {
+ return _address == trustedRecoveryAddress || _address == otherTrustedAddress;
+ }

By implementing these recommendations, the protocol can significantly mitigate the potential misuse of the recoverToken function and enhance the security and trust in the system.

Updates

Lead Judging Commences

inallhonesty Lead Judge 12 months ago
Submission Judgement Published
Invalidated
Reason: Non-acceptable severity

Support

FAQs

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