Description:
Impact: Low
Likelihood: High
While the MysteryBox::constructor() sets the initial values as seen here :
constructor() payable {
owner = msg.sender;
boxPrice = 0.1 ether;
@> 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));
}
The MysteryBox::openBox() function pushes those rewards with totally different values:
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;
}
Impact:
The average reward payout per user might be drastically different to what is expected by the protocol, since the actual values of the rewards are getting set by the MysteryBox::openBox() function and not Mysterybox::constructor as it might be expected.
Tools Used:
Manual Review and Forge.
Proof of Concept:
The following test and console output just proves that MysteryBox::openBox() is actually responsible for setting the reward values.
function testInconsistencyInRewardStructure() public {
vm.warp(1641070805);
address user = 0x1aC6F9601f2F616badcEa8A0a307e1A3C14767A4;
vm.deal(user, 0.2 ether);
vm.deal(address(mysteryBox), 0.1 ether);
vm.startPrank(user);
mysteryBox.buyBox{value: 0.1 ether}();
mysteryBox.openBox();
(string memory name, uint256 rewards) = mysteryBox.rewardsOwned(user, 0);
console.log(name, rewards);
vm.stopPrank();
console.log(user.balance);
}
The forementioned test produces this console output clearly stating that a Gold Coin has the value of 1000000000000000000 wei or 1 ether:
forge test --mt testInconsistencyInRewardStructure -vvv
[⠒] Compiling...
[⠢] Compiling 1 files with Solc 0.8.27
[⠆] Solc 0.8.27 finished in 1.05s
Compiler run successful!
Ran 1 test for test/TestMysteryBox.t.sol:MysteryBoxTest
[PASS] testInconsistencyInRewardStructure() (gas: 94154)
Logs:
Reward Pool Length: 4
@>Gold Coin 1000000000000000000
100000000000000000
Suite result: ok. 1 passed; 0 failed; 0 skipped; finished in 7.34ms (479.70µs CPU time)
Ran 1 test suite in 60.10ms (7.34ms CPU time): 1 tests passed, 0 failed, 0 skipped (1 total tests)
In this example it is obviously possible to win the highest value price with 9 addresses on given `block.timestamp`.
Recommended Mitigation:
Push the RewardsOwned() from Rewards[].
