Mystery Box

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

Updating state after external call in `MysteryBox::claimAllRewards` leads to a Re-entrancy vulnerability.

Summary

The function claimAllRewards() in the MysteryBox contract is vulnerable to a re-entrancy attack. Specifically, the function updates the contract’s state deleting rewardsOwned[msg.sender] after an external call is made to transfer funds to the user using payable(msg.sender).call. This creates an opportunity for a malicious contract to re-enter the function before the state is updated, allowing the attacker to drain the contract's funds.

Vulnerability Details

The following scenario demonstrates how an attacker can exploit this vulnerability:

  1. The attacker’s contract initiates a call to claimAllRewards().

  2. The external call to payable(msg.sender).call{value: totalValue} is made before the contract updates its state using delete rewardsOwned[msg.sender].

  3. The attacker’s contract re-enters claimAllRewards() before the state is updated and claims rewards again in the same transaction.

  4. This cycle repeats, allowing the attacker to withdraw more funds than they are entitled to, draining the contract of its funds.

A malicious contract might look like this:

contract Malicious {
MysteryBox public mysteryBox;
```Solidity
constructor(address mysteryBoxAddress) {
mysteryBox = MysteryBox(mysteryBoxAddress);
}
fallback() external payable {
// Re-enter claimAllRewards before state is updated
if (address(mysteryBox).balance > 0) {
mysteryBox.claimAllRewards();
}
}
function attack() public {
mysteryBox.claimAllRewards();
}
}
```
## Impact
An attacker can exploit this vulnerability by creating a malicious contract that repeatedly calls `claimAllRewards()` within the same transaction. By re-entering the function before the state is updated, the attacker could claim rewards multiple times, potentially draining all the available funds from the contract.
## Tools Used
Manual Review, unit-testing.
## Recommendations
There are two preventive measures that may be taken to mitigate reentrancy:
1. Carefully implement the checks->effects->interactions pattern throughout `MysteryBox` . In particular, make sure that it is not being violated within function calls. This ensures that all external calls are made after state updates.
2. Use a reentrancy guard, the best known of which is [OpenZeppelin’s ReentrancyGuard](https://github.com/OpenZeppelin/openzeppelin-contracts/blob/master/contracts/utils/ReentrancyGuard.sol), and apply it's modifier `nonReentrant()` on all public and external functions.
```Solidity
```
Updates

Appeal created

inallhonesty Lead Judge 11 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.