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

[M-01] Business logic error, new reward types are unobtainable by players due to hardcoded probabilities in `MysteryBox::openBox`

0xnelli

Summary

The owner of the contract has the ability to add new rewards via the MysteryBox::addReward function. However, any rewards added will not be obtainable by players of Mystery Box. This is due to the rewards distribution mechanism in the MysteryBox::openBox function being hard-coded specifically to the rewards set in the constuctor.

Vulnerability details

Examining the MysteryBox::openBox function below, it can be seen that rewards are allocated according to the if statements predicated on the randomly generated number value.

However, only the four original rewards that were set in the constructor are present and there is no mechanism to include the rewards added later by the owner using the MysteryBox::addReward function.

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));
} else if (randomValue < 95) {
// 20% chance to get Bronze Coin (75-94)
rewardsOwned[msg.sender].push(Reward("Bronze Coin", 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));
} else {
// 1% chance to get Gold Coin (99)
rewardsOwned[msg.sender].push(Reward("Gold Coin", 1 ether));
}
​
boxesOwned[msg.sender] -= 1;
}

Impact

Any new rewards added by the owner are unobtainable by the players of Mystery Box. Therefore the extensibility of the game is impacted, as the original rewards will be the only rewards that can every be claimed by the players.

Tools uses

Manual review

Recommended Mitigation

Rather than hardcoding the probabilities in the Mysterbox::openBox function, weight based probabilities can be used, whereby each MysteryBox::Reward will have a probability weight value. The probability of a reward being received will be the item's probability weight divided by the total probability weight of all rewards.

The update to the MysteryBox::Reward stuct will look as follows:

struct {
string name;
uint256 value;
uint256 probabilityWeight;
}

This tracks the weight of individual rewards, but as aforementioned, the total weight is also required for this selection method to work. It will be used as the new modulus value instead of the magic number value used currently in the MysteryBox::openBox function of 100.

This can be added as a state variable and this variable should be updated in both the MysteryBox::constructor and MysteryBox::addReward as these are the two functions in the contract that add new rewards.

This can be seen below:

uint256 totalProbabilityWeight;
​
...
constructor() payable {
owner = msg.sender;
boxPrice = 0.1 ether;
require(msg.value >= SEEDVALUE, "Incorrect ETH sent");
rewardPool.push(Reward("Gold Coin", 0.5 ether, 1));
rewardPool.push(Reward("Silver Coin", 0.25 ether, 4));
rewardPool.push(Reward("Bronze Coin", 0.1 ether, 20));
rewardPool.push(Reward("Coal", 0 ether, 75));
totalProbabilityWeight = 100;
}
​
function addReward(string memory _name, uint256 _value, uint256 _probabilityWeight) public {
require(msg.sender == owner, "Only owner can add rewards");
rewardPool.push(Reward(_name, _value, _weight));
totalProbabilityWeight += _weight;
}

MysteryBox::openBox now can be refactored to use probability weights to determine the distribution of rewards. This ensures it is dynamic and can accommodate future reward types that will be added.

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))) % totalProbabilityWeight;
​
// Determine the reward based on probability
uint256 cumulativeProbabilityWeight = 0;
Reward[] memory rewards = rewardPool;
​
for(uint256 i = 0; i < rewards.length; i++) {
cumulativeWeight += rewards[i].probabilityWeight;
​
if(randomValue < cumulativeWeight) {
rewardsOwned[msg.sender].push(rewards[i]);
boxesOwned[msg.sender] -= 1;
return;
}
}
}

As can be seen above, when new rewards are added to the reward pool, e.g. diamond coin as in TestMysteryBox.t.sol::testAddReward. MysteryBox::addReward("Diamond Coin", 2 ether, 1) now the totalProbabilityWeight is 101 and the chance of getting a diamond coin is 1/101, gold 1/101, silver 4/101, bronze 20/101 and coal 75/101. So the probabilities are updated accordingly and all rewards can now be earned by the players.

Updates

Appeal created

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

The rewards in constructor are different from the rewards in openBox

addReward won't have any effect on openBox

Support

FAQs

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

Give us feedback!