MyCut
First Flight #23
Beginner FriendlyFoundry
100 EXP
View results
Previous Next
Submission Details
Severity: low
Valid

Rounding error would result in zero amount been transfered when pot is closed

0x0noob

Source

https://github.com/Cyfrin/2024-08-MyCut/blob/main/src/Pot.sol#L54

Details

function closePot() external onlyOwner {
if (block.timestamp - i_deployedAt < 90 days) {
revert Pot__StillOpenForClaim();
}
if (remainingRewards > 0) {
uint256 managerCut = remainingRewards / managerCutPercent;
i_token.transfer(msg.sender, managerCut);
​
uint256 claimantCut = (remainingRewards - managerCut) / i_players.length;
for (uint256 i = 0; i < claimants.length; i++) {
_transferReward(claimants[i], claimantCut);
}
}
}

uint256 managerCut = remainingRewards / managerCutPercent could possibly return 0 when remainingRewards > managerCutPercent. The reason for this is that solidity does not return values in decimals, so if remainingRewards is say 4 and since managerCutPercent is 10, the result of managerCut will be zero. The implication fo this is that nothing will be transfered as intended when the pot is closed.

POC

function test_POC() public mintAndApproveTokens {
vm.startPrank(user);
contest = ContestManager(conMan).createContest(players, rewards, IERC20(ERC20Mock(weth)), 4);
ContestManager(conMan).fundContest(0);
vm.stopPrank();
​
vm.startPrank(player2);
Pot(contest).claimCut();
vm.stopPrank();
​
uint256 remainingRewards = Pot(contest).getRemainingRewards();
assertEq(remainingRewards, 3);
​
uint256 userBalanceBeforeClosingPot = (IERC20(ERC20Mock(weth)).balanceOf(conMan));
​
vm.warp(91 days);
​
vm.startPrank(user);
ContestManager(conMan).closeContest(contest);
vm.stopPrank();
​
uint256 finalBalance = (IERC20(ERC20Mock(weth)).balanceOf(conMan));
assertEq(finalBalance,userBalanceBeforeClosingPot);
}

finalBalance and userBalanceBeforeClosingPot is the same from the test above to prove that the balance of the owner does not change after pot is closed.

Tool Used

Manual Review

Recommendation

There are different ways to go about this. Protocol can add a require check that makes sure that remainingRewards is greater than managerCutPercent or that remainingRewards is a multiple of managerCutPercent which would mean that it introduce a variable to the mix just for the calculation but which does not influence the reward system. There are also many other solutions to this aside the aforementioned but the protocol decision should be based on its goal.

Updates

Lead Judging Commences

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

Dusty Pot

Support

FAQs

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