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

Risk of reentrancy attacks in DaiGoldAuction due to missing Checks-Effects-Interactions (CEI) Pattern in bid function

dianivanov

Summary

The DaiGoldAuction contract allows setting the bid token to any ERC20 token, assuming it has no internal taxes, fees, or callbacks. If a token with callback functions is used (intentially or not), unexpected behaviour can occur, because Checks-Effects-Interactions (CEI) pattern is not applied in the bid function of DaiGoldAuction contract.

Vulnerability Details

The DaiGoldAuction contract’s bid token can be set to any ERC20 token, and it relies on the assumption that the bid token has no internal taxes, fees, or callbacks. This assumption is not enforced programmatically, making the contract vulnerable to tokens with callback functions.

The current implementation of the bid function transfers the tokens before updating the state, which is not compliant with the CEI pattern:

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(); }
//@audit follow CEI in case of trasferFrom uses callbacks
> 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);
}

Without the CEI pattern, a token with a callback function could exploit the transfer and re-enter the contract, causing unexpected behavior.

This finding is similar to https://codehawks.cyfrin.io/c/2024-07-templegold/s/clyh2yrok0005bru09c788sjx, but with different unexpected outcome.

Impact

Using a token with callback functions as the bid token in the DaiGoldAuction contract can lead to reentrancy attacks. At first glance this can not lead to losses because depositors[msg.sender][epochIdCache] andinfo.totalBidTokenAmountare accrued, the picture would be different if it was a claim like function or deduction was the operation used, hence I believe the Impact is LOW and likelihood is MEDIUM. Hence the severity is M/L.

Tools Used

Manual Review

Recommendations

Use the Checks-Effects-Interactions (CEI) pattern or/and a reentrancy guard:

function bid(uint256 amount) external virtual override onlyWhenLive {
if (amount == 0) { revert CommonEventsAndErrors.ExpectedNonZero(); }
//@audit follow CEI in case of trasferFrom uses callbacks
- bidToken.safeTransferFrom(msg.sender, treasury, amount);
​
uint256 epochIdCache = _currentEpochId;
depositors[msg.sender][epochIdCache] += amount;
​
EpochInfo storage info = epochs[epochIdCache];
info.totalBidTokenAmount += amount;
+ bidToken.safeTransferFrom(msg.sender, treasury, amount);
emit Deposit(msg.sender, epochIdCache, amount);
}
Updates

Lead Judging Commences

inallhonesty Lead Judge over 1 year ago
Submission Judgement Published
Invalidated
Reason: Lack of quality

Support

FAQs

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