Mystery Box
First Flight #25
Beginner FriendlyFoundry
100 EXP
View results
Previous Next
Submission Details
Severity: high
Valid

[H-4] Reentrancy In `MysteryBox::claimAllRewards` can potentially end in loss of funds

JJS

Summary

The claimAllRewards function lets a user claim all of their rewards in one transaction. However, the fundamentals of writing function is Solidity is forgot in this function - The Checks-Effects-Interactions. Forgetting to do so can lead to reentrancy attack in the protocol.

Vulnerability Details

function claimAllRewards() public {
uint256 totalValue = 0;
for (uint256 i = 0; i < rewardsOwned[msg.sender].length; i++) {
totalValue += rewardsOwned[msg.sender][i].value;
}
require(totalValue > 0, "No rewards to claim");
​
@> (bool success,) = payable(msg.sender).call{value: totalValue}(""); <@
require(success, "Transfer failed");
​
delete rewardsOwned[msg.sender];

Impact

Reentrancy attacks are dangerous to protocols. Because the state is not yet updated. They can end up with drained contracts of funds! Consider the following scenario:

  1. User has bought a box.

  2. Then opened it.

  3. The user continued to claim all his rewards but has a malicious fallback/receive function.

  4. Because the state is not yet update (the mapping element is not yet deleted) there is an attack opening.

  5. His fallback/receive function calls back into claimAllRewards.

  6. The function sends them another payment.

  7. The attacker keeps on until they drain the funds out of the contract.

Tools Used

Manual Review

Recommendations

Follow CEI (Checks-Effects-Interactions) to prevent this attack:

function claimAllRewards() public {
uint256 totalValue = 0;
for (uint256 i = 0; i < rewardsOwned[msg.sender].length; i++) {
totalValue += rewardsOwned[msg.sender][i].value;
}
require(totalValue > 0, "No rewards to claim");
​
+ delete rewardsOwned[msg.sender];
- (bool success,) = payable(msg.sender).call{value: totalValue}("");
require(success, "Transfer failed");
​
+ (bool success,) = payable(msg.sender).call{value: totalValue}("");
- delete rewardsOwned[msg.sender];
}
Updates

Appeal created

inallhonesty Lead Judge 9 months ago
Submission Judgement Published
Validated
Assigned finding tags:

`claimAllRewards` reentrancy

Support

FAQs

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