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

`claimantCut` is calculated using all players causing claimants to receive fewer rewards

jfornells

Root + Impact

Pot::closePot calculates claimantCut divided by the total number of players instead of claimants, resulting in a much lower distributed amount and leaving tokens permanently trapped in the Pot.

Severity

High

Likelihood

High

Description

  • closePot() must calculate 10% of remainingRewards and transfer it to the owner. The rest must be divided and transferred among all players who claimed their reward via claimCut().

  • However, the amount of tokens that claimants should receive is calculated by dividing by the total players array length instead of the claimants array length. Since players.length is larger, claimants receive fewer tokens. This vulnerability also causes tokens to be permanently trapped in the Pot contract.

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);
}
}
}

Risk

Likelihood:

  • Occurs in every deployed Pot contract.

  • Every time the owner calls closePot().

Impact:

  • Claimants do not receive all the rewards they are entitled to.

  • By dividing remainingRewards by a larger number and sending that amount to fewer recipients than players, funds become trapped. For example, in the PoC case:
    A players array of length 4 with 1 ether per position. 3 players call claimCut(), resulting in a claimants array of length 3. remainingRewards = 1 ether. Owner takes 10%, leaving 9e17.
    9e17 / 4 = 225000000000000000
    This amount is transferred to the 3 claimants.
    Each claimant receives -> 225000000000000000
    Tokens trapped -> 225000000000000000 (25% of the distributable amount).
    The percentage of trapped tokens increases as the difference between players.length and claimants.length grows.

Proof of Concept

This test verifies through logs that claimants receive less than they are entitled to, and that funds remain trapped in the contract.

function test_Remaining_Rewards_Are_Divided_By_Players_Intead_Of_Claimants() public {
// There are 4 players, 1 weth each
// 3 players call claimCut()
address[3] memory pClaim = [player1, player3, player4];
for (uint256 i; i < pClaim.length; i++) {
vm.prank(pClaim[i]);
pot.claimCut();
}
uint256 rewardAmount = 1e18;
// Players have received their rewards
assertEq(weth.balanceOf(player1), rewardAmount);
assertEq(weth.balanceOf(player3), rewardAmount);
assertEq(weth.balanceOf(player4), rewardAmount);
// 1 weth remains to be split between owner and 4 players
uint256 remaningRewards = pot.getRemainingRewards();
assertEq(remaningRewards, 1e18);
// Advance time past 90 days to pass the check
vm.warp(block.timestamp + 90 days);
// Owner closes the pot
vm.prank(ownerContest);
contest.closeContest(address(pot));
// Owner has received 10%
uint256 amountToOwner = remaningRewards / 10;
assertEq(weth.balanceOf(address(contest)), amountToOwner);
// Correct amount claimants should have received
uint256 amountToPlayers = (remaningRewards - amountToOwner) / pClaim.length;
console.log("The players should have receided:", amountToPlayers);
console.log("Player1 has received............:", weth.balanceOf(player1) - rewardAmount);
console.log("Player3 has received............:", weth.balanceOf(player3) - rewardAmount);
console.log("Player4 has received............:", weth.balanceOf(player4) - rewardAmount);
​
uint256 amountReceived = weth.balanceOf(player1) - rewardAmount;
uint256 amountLost = amountToPlayers - amountReceived;
// Amount each player loses
console.log("Each player has lost............:", amountLost);
console.log("Tokens trapped in Pot..........:", weth.balanceOf(address(pot)));
}
Logs:
The players should have receided: 300000000000000000
Player1 has received............: 225000000000000000
Player3 has received............: 225000000000000000
Player4 has received............: 225000000000000000
Each player has lost............: 75000000000000000
Tokens trapped in Pot...........: 225000000000000000

Recommended Mitigation

Replace i_players.length with claimants.length.

function closePot() external onlyOwner {
...
​
- uint256 claimantCut = (remainingRewards - managerCut) / i_players.length;
+ uint256 claimantCut = (remainingRewards - managerCut) / claimants.length;
for (uint256 i = 0; i < claimants.length; i++) {
_transferReward(claimants[i], claimantCut);
}
}
}
Updates

Lead Judging Commences

ai-first-flight-judge Lead Judge about 3 hours ago
Submission Judgement Published
Validated
Assigned finding tags:

[H-02] Incorrect logic in `Pot::closePot` leads to unfair distribution to `claimants`, potentially locking the funds with no way to take that out

## Description in `closePot` function while calclulating the shares for claimaint cut, `i_players.length` is used, instead of `claimants.length`, causing low amount being distributed to claimants. ## Vulnerability Details [2024-08-MyCut/src/Pot.sol at main · Cyfrin/2024-08-MyCut (github.com)](https://github.com/Cyfrin/2024-08-MyCut/blob/main/src/Pot.sol#L57) `Pot::closePot` function is meant to be called once contest passed 90 days, it sends the owner cut to owner and rest is splitted among the users who claimed b/w 90 days period. However, current implementation is wrong.&#x20; It uses total users (i_players.length) instead of the users (claimants.length) who claimed during the duration. This creates an unfair distribution to the participants and some of the funds could be locked in the contract. In worst case scenerio, it could be 90% if nobody has claimed from the protocol during the 90 days duration. ## POC In existing test suite, add following test: ```solidity function testUnfairDistributionInClosePot() public mintAndApproveTokens { // Setup address[] memory testPlayers = new address[](3); testPlayers[0] = makeAddr("player1"); testPlayers[1] = makeAddr("player2"); testPlayers[2] = makeAddr("player3"); uint256[] memory testRewards = new uint256[](3); testRewards[0] = 400; testRewards[1] = 300; testRewards[2] = 300; uint256 testTotalRewards = 1000; // Create and fund the contest vm.startPrank(user); address testContest = ContestManager(conMan).createContest( testPlayers, testRewards, IERC20(ERC20Mock(weth)), testTotalRewards ); ContestManager(conMan).fundContest(0); vm.stopPrank(); // Only player1 claims their reward vm.prank(testPlayers[0]); Pot(testContest).claimCut(); // Fast forward 91 days vm.warp(block.timestamp + 91 days); // Record balances before closing the pot uint256 player1BalanceBefore = ERC20Mock(weth).balanceOf( testPlayers[0] ); // Close the contest vm.prank(user); ContestManager(conMan).closeContest(testContest); // Check balances after closing the pot uint256 player1BalanceAfter = ERC20Mock(weth).balanceOf(testPlayers[0]); // Calculate expected distributions uint256 remainingRewards = 600; // 300 + 300 unclaimed rewards uint256 ownerCut = remainingRewards / 10; // 10% of remaining rewards uint256 distributionPerPlayer = (remainingRewards - ownerCut) / 1; // as only 1 user claimed uint256 fundStucked = ERC20Mock(weth).balanceOf(address(testContest)); // actual results console.log("expected reward:", distributionPerPlayer); console.log( "actual reward:", player1BalanceAfter - player1BalanceBefore ); console.log("Fund stucked:", fundStucked); } ``` then run `forge test --mt testUnfairDistributionInClosePot -vv` in the terminal and it will show following output: ```js [⠊] Compiling... [⠒] Compiling 1 files with Solc 0.8.20 [⠘] Solc 0.8.20 finished in 1.63s Compiler run successful! Ran 1 test for test/TestMyCut.t.sol:TestMyCut [PASS] testUnfairDistributionInClosePot() (gas: 905951) Logs: User Address: 0x6CA6d1e2D5347Bfab1d91e883F1915560e09129D Contest Manager Address 1: 0x7BD1119CEC127eeCDBa5DCA7d1Bd59986f6d7353 Minting tokens to: 0x6CA6d1e2D5347Bfab1d91e883F1915560e09129D Approved tokens to: 0x7BD1119CEC127eeCDBa5DCA7d1Bd59986f6d7353 expected reward: 540 actual reward: 180 Fund stucked: 360 Suite result: ok. 1 passed; 0 failed; 0 skipped; finished in 1.58ms (506.33µs CPU time) ``` ## Impact Loss of funds, Unfair distribution b/w users ## Recommendations Fix the functions as shown below: ```diff 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; + uint256 totalClaimants = claimants.length; + if(totalClaimant == 0){ + _transferReward(msg.sender, remainingRewards - managerCut); + } else { + uint256 claimantCut = (remainingRewards - managerCut) / claimants.length; for (uint256 i = 0; i < claimants.length; i++) { _transferReward(claimants[i], claimantCut); } } + } } ```

Support

FAQs

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

Give us feedback!