MyCut

First Flight #23
Beginner FriendlyFoundry
100 EXP
View results
Submission Details
Severity: high
Valid

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

Summary

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)

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.

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:

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:

[⠊] 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

Tools Used

Manual Review, Foundry

Recommendations

Fix the functions as shown below:

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

Lead Judging Commences

equious Lead Judge 10 months ago
Submission Judgement Published
Validated
Assigned finding tags:

Incorrect distribution in closePot()

Support

FAQs

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