Summary
In the event that the Owner set the eggFindThreshold to 0, eggs will never be found. The eggFindThreshold is not that accurate.
Vulnerability Details
The logic of comparing the random against the eggFindThreshold is using the < (less than) operation, meaning if the right and left value are the same, it will then return false.
Impact
If the threshold become 0, then no player could ever find eggs due to the logic implemented of random < eggFindThreshold.
PoS
Consider the Scenario below,
Scenario 1:
Owner set eggFindThreshold to 0
If player get random of 0, the condition if ( random < eggFindThreshold) will become false, egg will be impossible to be found.
Scenario 2:
Owner wants to set the eggFindThreshold to 1%, according to the comment in EggHuntGame.sol, 20% rate is 20.
The logic will then consider 1 < 1 and will return false, making the rate is inaccurate.
PoC
Modify the EggHuntGame.sol::searchForEgg() random variable to always become 0 before running the test so that we will always random value of 0.
function searchForEgg() external {
. . .
uint256 random = 0;
. . .
}
Add the following test to EggGameTest contract:
function test_noMinimalThresholdEnforced() public {
uint256 minimumThreshold = game.eggFindThreshold();
console.log("Minimum Threshold: ", minimumThreshold);
game.startGame(200);
assertEq(game.gameActive(), true);
game.setEggFindThreshold(0);
uint256 newThreshold = game.eggFindThreshold();
console.log("New Threshold: ", newThreshold);
console.log("Random at 0 : ", 0 < 0 ? true : false);
vm.prank(attacker);
game.searchForEgg();
console.log("Attacker's egg count: ", game.eggsFound(address(attacker)));
console.log("Total Egg count : ", game.eggCounter());
}
Run with the following command:
forge test --mt test_noMinimalThresholdEnforced -vvv
Result:
Ran 1 test for test/EggHuntGameTest.t.sol:EggGameTest
[PASS] test_noMinimalThresholdEnforced() (gas: 85841)
Logs:
Minimum Threshold: 20
New Threshold: 0
Random at 0 : false
Attacker's egg count: 0
Total Egg count : 0
Tools Used
Recommendations
To address this issue, the condition for EggHuntGame::searchForEgg() need to be adjusted so that there are always chance of winning for the player. Second, to increase accuracy and clarity, the random value can be always be added 1 since the minimum of the random modulo by 100 is always zero (0) to 99, adding one make the range after the modulo between 1 - 100 (more accurate). To address this, the changes below can be implemented.
/// @notice Chance (in percent) to find an egg on each search attempt.
uint256 public eggFindThreshold = 20; // Default is a 20% chance
. . .
function setEggFindThreshold(uint256 newThreshold) external onlyOwner {
require(newThreshold <= 100, "Threshold must be <= 100");
+ require(newThreshold >= 1, "Threshold must be >= 1");
eggFindThreshold = newThreshold;
}
. . .
function searchForEgg() external {
require(gameActive, "Game not active");
require(block.timestamp >= startTime, "Game not started yet"); // this one ok!
require(block.timestamp <= endTime, "Game ended"); // this also ok!
uint256 random = uint256(
keccak256(abi.encodePacked(block.timestamp, block.prevrandao, msg.sender, eggCounter))
) % 100;
+ // ensure that random is always between 1 - 100.
+ random += 1;
- if (random < eggFindThreshold) {
+ // ensure that the comparison is always less or equal to random
+ if (random <= eggFindThreshold) {
eggCounter++;
eggsFound[msg.sender] += 1;
eggNFT.mintEgg(msg.sender, eggCounter);
emit EggFound(msg.sender, eggCounter, eggsFound[msg.sender]);
}
}
To test this fix method, we can run the test again using the same condition where we hardcoded the random to 0 and change the eggFindThreshold to the minimum value of 1. This is the new test to add to test the fix
function test_noMinimalThresholdEnforced() public {
uint256 minimumThreshold = game.eggFindThreshold();
console.log("Minimum Threshold: ", minimumThreshold);
game.startGame(200);
assertEq(game.gameActive(), true);
game.setEggFindThreshold(1);
uint256 newThreshold = game.eggFindThreshold();
console.log("New Threshold: ", newThreshold);
console.log("Example: we got 100 % 100 = 0 for our value");
console.log(" then it will add 1 to maintain precision");
console.log("Random at 0 : ", 1 <= 1 ? true : false);
vm.prank(attacker);
game.searchForEgg();
console.log("Attacker's egg count: ", game.eggsFound(address(attacker)));
console.log("Total Egg count : ", game.eggCounter());
}
Running the test will give this output:
Ran 1 test for test/EggHuntGameTest.t.sol:EggGameTest
[PASS] test_noMinimalThresholdEnforced() (gas: 211268)
Logs:
Minimum Threshold: 20
New Threshold: 1
Example: we got 100 % 100 = 0 for our value
then it will add 1 to maintain precision
Random at 0 : true
Attacker's egg count: 1
Total Egg count : 1
