Summary
The MysteryBox::claimAllRewards function iterates through every reward owned by a player, which may consume an excessive amount of gas, should this reward list grow too large. This can result in a denial or service, due to out-of-gas errors.
Vulnerability Details
As can be seen below, MysteryBox::claimAllRewards iterates through every Reward owned by the player.
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];
}
This includes the Coal reward type, which has a value of 0 ether and therefore bloats the for loop with unnecessary iterations. This is highlighted in the code excerpt below:
function openBox() public {
require(boxesOwned[msg.sender] > 0, "No boxes to open");
uint256 randomValue = uint256(keccak256(abi.encodePacked(block.timestamp, msg.sender))) % 100;
@> if (randomValue < 75) {
@> rewardsOwned[msg.sender].push(Reward("Coal", 0 ether));
} else if (randomValue < 95) {
rewardsOwned[msg.sender].push(Reward("Bronze Coin", 0.1 ether));
} else if (randomValue < 99) {
rewardsOwned[msg.sender].push(Reward("Silver Coin", 0.5 ether));
} else {
rewardsOwned[msg.sender].push(Reward("Gold Coin", 1 ether));
}
boxesOwned[msg.sender] -= 1;
}
Therefore, should a player allow the rewardsOwned list to grow to large, by buying and opening a large number of mystery boxes without claiming the underlying rewards, they may encounter out-of-gas errors, resulting in a denial of service.
Impact
Players with a large amount of rewardsOwned may not be able to claim all their rewards in one transaction using the MysteryBox::claimAllRewards function as intended.
Tools used
Manual review
Recommended Mitigation
An additional mapping should be added to the MysteryBox contract that maps the address of the player to the value accrued by all their rewards owned. The MysteryBox::openBox function should update this mapping, which can be used rather than looping through an array of the rewards owned by the player.
These changes can be observed below:
mapping(address => uint256) totalValueEarned;
...
function openBox() public {
require(boxesOwned[msg.sender] > 0, "No boxes to open");
// Generate a random number between 0 and 99
uint256 randomValue = uint256(keccak256(abi.encodePacked(block.timestamp, msg.sender))) % 100;
// Determine the reward based on probability
if (randomValue < 75) {
// 75% chance to get Coal (0-74)
rewardsOwned[msg.sender].push(Reward("Coal", 0 ether));
// no need to update as reward is worth 0 eth
} else if (randomValue < 95) {
// 20% chance to get Bronze Coin (75-94)
rewardsOwned[msg.sender].push(Reward("Bronze Coin", 0.1 ether));
+ totalValueEarned[msg.sender] += 0.1 ether;
} else if (randomValue < 99) {
// 4% chance to get Silver Coin (95-98)
rewardsOwned[msg.sender].push(Reward("Silver Coin", 0.5 ether));
+ totalValueEarned[msg.sender] += 0.5 ether;
} else {
// 1% chance to get Gold Coin (99)
rewardsOwned[msg.sender].push(Reward("Gold Coin", 1 ether));
+ totalValueEarned[msg.sender] += 1 ether;
}
boxesOwned[msg.sender] -= 1;
}
...
function claimAllRewards() public {
uint256 totalValue = totalValueEarned[msg.sender];
require(totalValue > 0, "No rewards to claim");
(bool success,) = payable(msg.sender).call{value: totalValue}("");
require(success, "Transfer failed");
delete rewardsOwned[msg.sender];
+ totalValueEarned[msg.sender] = 0;
}
