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

Potential discrepancy in bidToken transfers due to missing fee check

dianivanov

Summary

Unlike the SpiceAuction contract, which includes transaction fee checks for the bid token, the DaiGoldAuction contract lacks such checks. This absence can lead to discrepancies in the expected token amounts during transfers, as it relies on the assumption that the bidToken has no funky fees or taxes. This oversight can cause financial discrepancies and potential losses for the protocol because the bidToken is transferred to the DaiGoldAuctioncontract itself in the bid function.

Vulnerability Details

In the documentation, theDaiGoldAuction contract's bidToken is assumed to have no internal taxes, fees, or callbacks:

"Bid token can be updated later using setBidToken. It can be assumed that the bid token has no funky internal taxes or fees, callbacks or complex functionalities beyond the usual OZ ERC20 functions."

This assumption is documented but not enforced programmatically:

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

function bid(uint256 amount) external virtual override onlyWhenLive {
if (amount == 0) { revert CommonEventsAndErrors.ExpectedNonZero(); }
bidToken.safeTransferFrom(msg.sender, treasury, amount);
​
uint256 epochIdCache = _currentEpochId;
depositors[msg.sender][epochIdCache] += amount;
​
EpochInfo storage info = epochs[epochIdCache];
info.totalBidTokenAmount += amount;
emit Deposit(msg.sender, epochIdCache, amount);
}

As seen in the code of the bidfunction in DaiGoldAuctioncontract the info.totalBidTokenAmountis updated based on the amount (this value is used to decide the rewards later in the claim function), but the actual amount received can be less if bidTokenhas fees. This discrepancy can lead to losses for the protocol or other unexpected issues, if bidToken has fees (accidentially or not).

The SpiceAuction contract includes a check to verify that the transferred amount matches the expected amount, ensuring no transfer fees or other deductions occur:

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

function bid(uint256 amount) external virtual override {
...
uint256 _bidTokenAmountBefore = IERC20(bidToken).balanceOf(_recipient);
IERC20(bidToken).safeTransferFrom(msg.sender, _recipient, amount);
uint256 _bidTokenAmountAfter = IERC20(bidToken).balanceOf(_recipient);
// fee on transfer tokens
if (amount != _bidTokenAmountAfter - _bidTokenAmountBefore) { revert CommonEventsAndErrors.InvalidParam(); }
...
}

This ensures that the transferred amount matches the expected amount, preventing issues with tokens that have transfer fees.

Same check has to be applied in DaiGoldAuction.

Bear in mind thatbidToken can't be reset before the epoch ends (this can be disturbing if update is required to avoid token with fees):

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

function setBidToken(address _bidToken) external override onlyElevatedAccess {
if (_bidToken == address(0)) { revert CommonEventsAndErrors.InvalidAddress(); }
> if (!epochs[_currentEpochId].hasEnded()) { revert InvalidOperation(); }
bidToken = IERC20(_bidToken);
emit BidTokenSet(_bidToken);
}

Impact

If a token with transfer fees is used as the bid token, the DaiGoldAuction may receive less than expected in it's bid function, leading to potential financial losses. Additionally, it will be impossible to change the bidToken once users start using it, as they would have to wait for the current epoch to end.

It seems like likeihood is Medium because literally "It might occur under specific conditions. For example, a peculiar ERC20 token is used on the platform." In the context of the protocl token with funky fees is considered peculiar.

The impact seems like a high because the protocol can lose some assets because of the potential taxes.

Hence the severity is High.

Tools Used

Manual Review

Recommendations

DaiGoldAuction contract has to apply the same check like the SpiceAuctioncontract, to ensure the bid function can't transfer bid tokens with fees:

function bid(uint256 amount) external virtual override onlyWhenLive {
if (amount == 0) { revert CommonEventsAndErrors.ExpectedNonZero(); }
- bidToken.safeTransferFrom(msg.sender, treasury, amount);
+. _bidTokenAmountBefore = bidToken.balanceOf(treasury);
+ bidToken.safeTransferFrom(msg.sender, treasury, amount);
+ _bidTokenAmountAfter = bidToken.balanceOf(treasury);
+ if (amount != _bidTokenAmountAfter - _bidTokenAmountBefore) { revert CommonEventsAndErrors.InvalidParam(); }
.
uint256 epochIdCache = _currentEpochId;
depositors[msg.sender][epochIdCache] += amount;
​
EpochInfo storage info = epochs[epochIdCache];
info.totalBidTokenAmount += amount;
emit Deposit(msg.sender, epochIdCache, amount);
}
Updates

Lead Judging Commences

inallhonesty Lead Judge over 1 year ago
Submission Judgement Published
Invalidated
Reason: Known issue

Appeal created

dianivanov Submitter
over 1 year ago
inallhonesty Lead Judge
over 1 year ago
inallhonesty Lead Judge over 1 year ago
Submission Judgement Published
Invalidated
Reason: Known issue

Support

FAQs

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