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

Incorrect Rewards for Silver and Gold Coins in `openBox`.

v1vah0us3

Vulnerability Details

In the MysteryBox contract, there is an inconsistency between the reward values initialized in the constructor and the reward values allocated in the openBox function. Specifically, the rewards for Silver Coin and Gold Coin in the openBox function are incorrectly doubled compared to the values assigned in the contract's constructor.

In the constructor, the reward values are initialized as follows:

@> rewardPool.push(Reward("Gold Coin", 0.5 ether));
@> rewardPool.push(Reward("Silver Coin", 0.25 ether));
rewardPool.push(Reward("Bronze Coin", 0.1 ether));
rewardPool.push(Reward("Coal", 0 ether));

Gold Coin is valued at 0.5 ether. Silver Coin is valued at 0.25 ether.

However, in the openBox function, the rewards are distributed with doubled values:

} 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));
}
...........

The Silver Coin is incorrectly assigned a value of 0.5 ether (instead of the intended 0.25 ether).
The Gold Coin is incorrectly assigned a value of 1 ether (instead of the intended 0.5 ether).

Impact

This inconsistency significantly increases the reward value for both the Silver and Gold Coin compared to the expected values set in the constructor. As the result the contract will pay out twice the intended value for both Silver and Gold rewards. This could drain the contract's balance, especially if randomness is manipulated (as highlighted in a separate medium-severity issue regarding possible randomness manipulation).

Tools Used

Manual review, Visual Studio Code (VSCode)

Recommendations

To address this vulnerability, the reward values in the openBox function should be corrected to match the intended values initialized in the constructor. The following code change should be implemented:

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));
+ rewardsOwned[msg.sender].push(Reward("Silver Coin", 0.25 ether));
} else {
// 1% chance to get Gold Coin (99)
- rewardsOwned[msg.sender].push(Reward("Gold Coin", 1 ether));
+ rewardsOwned[msg.sender].push(Reward("Gold Coin", 0.5 ether));
}
​
boxesOwned[msg.sender] -= 1;
}
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

Support

FAQs

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

Give us feedback!