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

Reentrancy attack in `MysteryBox::claimAllRewards` allows attacker to drain contract balance

pob001

Relevant GitHub Links

https://github.com/Cyfrin/2024-09-mystery-box/blob/main/src/MysteryBox.sol#L79-L90

Summary

The MysteryBox::claimAllRewards function does not follow the CEIFREI-PI principles, and, as a result, enables participants to drain the contract balance.

In the current implementation of MysteryBox::claimAllRewards, rewards are transferred first, and only then is rewardsOwned[msg.sender] reset.

A malicious player who has participated in the prediction could have a fallback or receive function that triggers the MysteryBox::claimAllRewards function again, allowing them to claim multiple refunds. They could repeat this process until the contract’s balance is completely drained.

Impact

All the funds paid by players could be stolen by a malicious actor.

Recommendations

To resolve this issue, the MysteryBox::claimAllRewards function should update the rewardsOwned array before making any external calls.

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");
​
- delete rewardsOwned[msg.sender];
}
Updates

Appeal created

inallhonesty Lead Judge about 1 year 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.