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

Reentrancy Vulnerability in `claimAllRewards` and `claimSingleReward`.

v1vah0us3

Vulnerability Details

Both the claimAllRewards and claimSingleReward functions are vulnerable to reentrancy attacks due to the lack of the Checks-Effects-Interactions pattern. This can allow a malicious actor to repeatedly call one of these functions before state changes are fully applied, potentially draining the contract's funds.

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];
}
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");
​
@> (bool success,) = payable(msg.sender).call{value: value}("");
require(success, "Transfer failed");
​
@> delete rewardsOwned[msg.sender][_index];
}

Both functions transfer ETH to the user before updating the state of the contract (before deleting or modifying the rewardsOwned array). This violates the Checks-Effects-Interactions pattern, which is crucial to preventing reentrancy attacks.
A malicious user could craft a contract that calls claimAllRewards or claimSingleReward. Once the call to a malicious contract is made, before the delete operation is executed, the attacker’s fallback function would make another call to the same reward function in MysteryBox.sol, continuing to claim funds. This process would continue until the contract is drained.

Impact

The contract’s entire balance could be drained by an attacker.
If exploited, this attack could lead to a complete loss of all funds stored in the contract.

Tools Used

Manual review, Visual Studio Code (VSCode)

Recommendations

To mitigate this vulnerability, the contract should adopt the Checks-Effects-Interactions pattern, ensuring that all state changes occur before making external calls. Specifically, the rewardsOwned array should be cleared before transferring any funds. You may also want to use the ReentrancyGuard modifier from OpenZeppelin’s library to prevent reentrant calls to the contract.

Proposed Fix for claimAllRewards:

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}("");
+ delete rewardsOwned[msg.sender];
+ (bool success,) = payable(msg.sender).call{value: totalValue}("");
require(success, "Transfer failed");
​
- delete rewardsOwned[msg.sender];
}

Proposed Fix for claimSingleReward:

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");
​
- (bool success,) = payable(msg.sender).call{value: value}("");
+ 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:

`claimAllRewards` reentrancy

`claimSingleReward` reentrancy

Support

FAQs

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

Give us feedback!