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

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

pob001

Relevant GitHub Links

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

Summary

The MysteryBox::claimSingleReward 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::claimSingleReward, rewards are transferred first, and only then is rewardsOwned[msg.sender][_index] reset.

A malicious player who has participated in the prediction could have a fallback or receive function that triggers the MysteryBox::claimSingleReward 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::claimSingleReward function should update the rewardsOwned array before making any external calls:

function claimSingleReward(uint256 _index) public {
require(_index <= rewardsOwned[msg.sender].length, "Invalid index");
uint256 value = rewardsOwned[msg.sender][_index].value;
require(value > 0, "No reward to claim");
+ delete rewardsOwned[msg.sender][_index];
​
(bool success,) = payable(msg.sender).call{value: value}("");
require(success, "Transfer failed");
​
- delete rewardsOwned[msg.sender][_index];
}
Updates

Appeal created

inallhonesty Lead Judge about 1 year ago
Submission Judgement Published
Validated
Assigned finding tags:

`claimSingleReward` reentrancy

Support

FAQs

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