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

Faulty math in Pot::closePot() can lead to a loss of funds if not all players make claims.

ollam

Summary

If any player does not make their claim on a pot, the portion of funds that should have been distrubuted to those who made claims will be lost with no way to recover the tokens.

Vulnerability Details

According to the docs, when a pot is closed the remainder of the pool (less a cut for the manager) should be distributed between all users who have made claims. However, the math used to calculate claimantCut uses i_players.length instead of claimants.length. This leaves money on the table with no way to rescue funds.

@> uint256 claimantCut = (remainingRewards - managerCut) / i_players.length;
for (uint256 i = 0; i < claimants.length; i++) {
_transferReward(claimants[i], claimantCut);
}

1. Consider a pot of Weth with 5 users each allocated 1 Weth of rewards.

2. Assume 4 users made claims, but one did not.

3. At the end of the pot, 1 Weth is remaining. The manager takes 0.1 Weth (10%) and the reminder is split 5 ways (0.18 Weth each) but only distributed 4 ways, leaving 0.18 Weth in the pot. This does not follow the documentation, and there is nowhere in the contract that would allow for the rescue of these funds.

PoC - paste this into TestMyCut.t.sol:

function testStuckFunds() public {
vm.startPrank(user);
weth = new ERC20Mock("WETH", "WETH", user, 5e18);
address player1 = makeAddr("player1");
address player2 = makeAddr("player2");
address player3 = makeAddr("player3");
address player4 = makeAddr("player4");
address player5 = makeAddr("player5");
address[] memory players = new address[](5);
players[0] = player1;
players[1] = player2;
players[2] = player3;
players[3] = player4;
players[4] = player5;
uint256[] memory rewards = new uint256[](5);
rewards[0] = 1e18;
rewards[1] = 1e18;
rewards[2] = 1e18;
rewards[3] = 1e18;
rewards[4] = 1e18;
​
address pot = ContestManager(conMan).createContest(players, rewards, IERC20(ERC20Mock(weth)), 5e18);
weth.approve(conMan, 5e18);
ContestManager(conMan).fundContest(0);
​
vm.startPrank(player1);
Pot(pot).claimCut();
vm.startPrank(player2);
Pot(pot).claimCut();
vm.startPrank(player3);
Pot(pot).claimCut();
vm.startPrank(player4);
Pot(pot).claimCut();
​
vm.warp(91 days);
​
vm.startPrank(user);
ContestManager(conMan).closeContest(pot);
​
assert(weth.balanceOf(pot) == 1.8e17);
}

Impact

Funds that should have been distributed amongst claimants will be left in the contract.

Tools Used

Manual review

Recommendations

Use claimants.length instead of i_players.length for the calculation of claimantCut.

- 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

equious Lead Judge 12 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.