Submission Details
Severity:
Mismatch of reward values in `MysteryBox::constructor` vs `MysteryBox::openBox`
Description:
The constructor initializes 4 types of rewards along with their names and values. The function openBox however assigns different values to the silver and gold reward types.
// constructor
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));
// openBox
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)); // @audit mismatch -> constructor has 0, 0.1, 0.25, 0.5
}
Impact:
Users with gold or silver rewards may expect 0.5 and 0.25 ETH respectively and end up with 1.0 or 0.5 ETH, or vice versa. Basically, users could be confused on what the true value of their rewards are.
Proof of Concept:
function testRewardValuesDiffer() public {
uint256 timestamp = block.timestamp;
// Brute-force approach to find the desired address
for (uint256 i = 0; i < 1000; i++) {
address testAddress = address(uint160(i));
uint256 randomValue = uint256(keccak256(abi.encodePacked(timestamp, testAddress))) % 100;
if (randomValue == 95) { // desired testAddress (msg.sender) to get silver reward
vm.startPrank(testAddress);
vm.deal(testAddress, 1 ether);
mysteryBox.buyBox{value: 0.1 ether}();
mysteryBox.openBox();
MysteryBox.Reward[] memory rewards = mysteryBox.getRewards();
// [Return] [Reward({ name: "Silver Coin", value: 500000000000000000 [5e17] })]
console2.log(rewards[0].value);
vm.stopPrank();
}
}
}
So the actual reward values come from openBox, which will override the values set in the constructor.
Recommended Mitigation:
Update the values of the rewards in either the constructor or in openBox, so that values match.
+ // example updating constructor to match openBox
- rewardPool.push(Reward("Gold Coin", 0.5 ether));
- rewardPool.push(Reward("Silver Coin", 0.25 ether));
+ rewardPool.push(Reward("Gold Coin", 1.0 ether));
+ rewardPool.push(Reward("Silver Coin", 0.5 ether));
rewardPool.push(Reward("Bronze Coin", 0.1 ether));
rewardPool.push(Reward("Coal", 0 ether));
Updates
Appeal created
inallhonesty about 1 year ago
Submission Judgement Published Assigned finding tags:
The rewards in constructor are different from the rewards in openBox
Rewards breakdown
nSLOC
86This contest has a flat reward structure with the following payouts:
100 EXP
20 EXP
2 EXP
Live
The contest is live. Earn rewards by submitting a finding.
Appeals
This is your time to appeal against judgements on your submissions.
Appeals Review
Appeals are being carefully reviewed by our judges.