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

Attacker Can Drain All Funds from the `MysteryBox` Contract

badal_sharma09

Description

The MysteryBox#withdrawFunds() function allows the owner to withdraw all the ETH stored in the contract, including the funds intended for user rewards. While the function checks that the caller is the owner, it does not differentiate between the contract's operational funds and the reward pool, allowing the owner to withdraw the entire contract balance. This leaves users, who have earned rewards but have not yet claimed them, unable to withdraw their rightful rewards, as the contract's balance would be drained.

function withdrawFunds() public {
require(msg.sender == owner, "Only owner can withdraw");
// Owner withdraws all the contract's balance, including rewards pool
(bool success,) = payable(owner).call{value: address(this).balance}("");
require(success, "Transfer failed");
}

Impact

The contract owner can withdraw all the ETH from the contract by calling withdrawFunds(), depleting the funds needed for user rewards. As a result, users who have won rewards but have not claimed them will be unable to withdraw their winnings, leading to a complete loss of trust in the system and financial loss for participants.

Proof of Concept

  1. The MysteryBox the contract starts with a balance of 1 ETH.

  2. Hundreds of users buy mystery boxes and open them, generating rewards like Coal, Bronze Coin, Silver Coin, or Gold Coin.

  3. The contract's balance grows to 1000 ETH, but users have not yet claimed their rewards.

  4. The owner then calls withdrawFunds() and drains the entire 1000 ETH from the contract.

  5. Now, the contract has a zero balance, and users who have unclaimed rewards are left with nothing, as there are no funds left to honour their reward withdrawals.

Proof of Concept (Code Test)

function test_OwnerWithdrawAllBalance() external {
uint256 newPrice = 0.1 ether;
assertEq(address(mysteryBox).balance, 0.1 ether);
assertEq(address(owner).balance, 0);
​
// Owner sets a new price for boxes
vm.prank(address(owner));
mysteryBox.setBoxPrice(newPrice);
​
// User1 buys and opens a box
vm.startPrank(user1);
vm.deal(address(user1), 0.1 ether);
mysteryBox.buyBox{value: 0.1 ether}();
mysteryBox.openBox();
vm.stopPrank();
​
// User2 buys and opens a box
vm.startPrank(user2);
vm.deal(address(user2), 0.1 ether);
mysteryBox.buyBox{value: 0.1 ether}();
mysteryBox.openBox();
vm.stopPrank();
​
// Check rewards and contract balance before withdrawal
vm.prank(user1);
MysteryBox.Reward[] memory reward = mysteryBox.getRewards();
​
vm.prank(user2);
mysteryBox.getRewards();
​
assertEq(address(mysteryBox).balance, 0.3 ether);
assertEq(address(owner).balance, 0);
​
// Owner withdraws all funds
vm.prank(owner);
mysteryBox.withdrawFunds();
assertEq(address(mysteryBox).balance, 0);
assertEq(address(owner).balance, 0.3 ether);
​
// User1 tries to claim rewards, but the contract has no funds
vm.startPrank(user1);
vm.expectRevert("Transfer failed");
mysteryBox.claimAllRewards();
}

For testing purposes, update this line since the Coal prize does not provide any significant reward, and the chances of getting better rewards are very low.

// File src/mystery.sol#openBox
rewardsOwned[msg.sender].push(Reward("Coal", 0.1 ether));

Recommendation

To prevent the owner from draining the reward pool, it is recommended to:

  1. Separate Operational Funds and Reward Pool: Implement logic to maintain distinct balances for operational funds and user rewards.

  2. Restrict withdrawFunds(): Ensure that the owner can only withdraw operational funds and not funds set aside for rewards.

Updates

Appeal created

inallhonesty Lead Judge about 1 year ago
Submission Judgement Published
Invalidated
Reason: Non-acceptable severity

Support

FAQs

Can't find an answer? Chat with us on Discord, Twitter or Linkedin.

Give us feedback!