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

Incorrect Manager Cut Calculation Due to Inverted Operation Order

ognjenovicuros

Description

  • The closePot() function calculates the manager's cut from remaining unclaimed rewards using a fixed percentage defined by managerCutPercent = 10.

  • The current implementation uses remainingRewards / managerCutPercent which performs division before multiplication. This approach accidentally produces the correct result only because the constant happens to be 10 (meaning divide by 10 = 10%), but it violates proper order of operations and causes precision loss. The correct formula should multiply first, then divide to preserve precision: remainingRewards * managerCutPercent / 100.

function closePot() external onlyOwner {
if (block.timestamp - i_deployedAt < 90 days) {
revert Pot__StillOpenForClaim();
}
if (remainingRewards > 0) {
// @> Wrong order: divides first instead of multiplying
uint256 managerCut = remainingRewards / managerCutPercent;
i_token.transfer(msg.sender, managerCut);

Risk

Likelihood:

  • This code executes when the pot is closed after 90 days with unclaimed rewards remaining.

  • The bug remains unnoticed because dividing by 10 happens to equal 10%, making the mathematical coincidence mask the underlying logic error.

Impact:

  • If managerCutPercent is ever changed from 10 to a different value (e.g., 15, 20, 25), the calculation breaks and produces incorrect distributions.

  • Integer division causes precision loss; for example, remainingRewards = 99 tokens and managerCutPercent = 10 gives 99 / 10 = 9 tokens instead of the intended percentage-based calculation.

  • The hardcoded constant makes the code inflexible; if governance or future upgrades require adjusting the percentage, the inverted logic would need correction anyway.

Proof of Concept

// Current implementation with managerCutPercent = 10
uint256 remainingRewards = 1000e18;
uint256 managerCut = remainingRewards / 10; // = 100e18 (happens to be correct)
​
// If percentage changes to 15:
uint256 managerCutPercent = 15;
uint256 managerCut = remainingRewards / 15; // = 66.67e18 (WRONG! Should be 150e18)
​
// Precision loss example:
uint256 remainingRewards = 99;
uint256 managerCut = 99 / 10; // = 9 (loses 0.9 in integer division)
uint256 correct = 99 * 10 / 100; // = 9 (same result but wrong approach)

Recommended Mitigation

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

Lead Judging Commences

ai-first-flight-judge Lead Judge 11 days ago
Submission Judgement Published
Validated
Assigned finding tags:

[L-03] [H-03] Precision loss can lead to rewards getting stuck in the pot forever

### \[H-03] Precision loss can lead to rewards getting stuck in the pot forever **Description:** When contest manager closes the pot by calling `Pot::closePot`, 10 percent of the remaining rewards are transferred to the contest manager and the rest are distributed equally among the claimants. It does this by dividing the rewards by the manager's cut percentage which is 10. Then the remaining rewards are divided by the number of players to distribute equally among claimants. Since solidity allows only integer division this will lead to precision loss which will cause a portion of funds to be left in the pot forever. Each pot follows the same method, so as number of pots grow, the loss of funds is very significant. **Impact:** Reward tokens get stuck in the pot forever which causes loss of funds. **Proof of code:** Add the below test to `test/TestMyCut.t.sol` ```javascript function testPrecisionLoss() public mintAndApproveTokens { ContestManager cm = ContestManager(conMan); uint playersLength = 3; address[] memory p = new address[](playersLength); uint256[] memory r = new uint256[](playersLength); uint tr = 86; p[0] = makeAddr("_player1"); p[1] = makeAddr("_player2"); p[2] = makeAddr("_player3"); r[0] = 20; r[1] = 23; r[2] = 43; vm.startPrank(user); address pot = cm.createContest(p, r, weth, tr); cm.fundContest(0); vm.stopPrank(); console.log("\n\ntoken balance in pot before: ", weth.balanceOf(pot)); vm.prank(p[1]); // player 2 Pot(pot).claimCut(); vm.prank(p[0]); // player 1 Pot(pot).claimCut(); vm.prank(user); vm.warp(block.timestamp + 90 days + 1); cm.closeContest(pot); console.log( "\n\ntoken balance in pot after closing pot: ", weth.balanceOf(pot) ); assert(weth.balanceOf(pot) != 0); } ``` Run the below test command in terminal ```Solidity forge test --mt testPrecisionLoss -vv ``` Which results in the below output ```Solidity [⠒] Compiling... [⠆] Compiling 1 files with 0.8.20 [⠰] Solc 0.8.20 finished in 2.57s Compiler run successful! Ran 1 test for test/TestMyCut.t.sol:TestMyCut [PASS] testPrecisionLoss() (gas: 936926) Logs: token balance in pot before: 86 token balance in pot after closing pot: 1 Suite result: ok. 1 passed; 0 failed; 0 skipped; finished in 1.75ms (654.60µs CPU time) Ran 1 test suite in 261.16ms (1.75ms CPU time): 1 tests passed, 0 failed, 0 skipped (1 total tests) ``` If you observe the output you can see the pot still has rewards despite distributing them to claimants. **Recommended Mitigations:** Fixed-Point Arithmetic: Utilize a fixed-point arithmetic library or implement a custom solution to handle fee calculations with greater precision.

Support

FAQs

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

Give us feedback!