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

User can claim his cut from the contest after the contest is closed and the period is ended

FindingNem0

Summary

user can call pot::claimCut after the owner called pot::closeContest and calim his cut and be eligible for distrbuting remmaining rewards

https://github.com/Cyfrin/2024-08-MyCut/blob/946231db0fe717039429a11706717be568d03b54/src/Pot.sol#L37

Vulnerability Details

If a user found this flow he will have advantage over other users how forgot to claim their cut .since he can claim his cut anytime even if the owner called pot::closeContest.therefore the player will be pushed into the claimants array and if the owner called pot::closeContest again he will receive another cut and the user who found the flow will recieve extra reward from the remmaining tokens even he considered inelligible according to protocl docs

ExploitSceanario

  • The following scenario demonstrate that player2 and player3 discovered the flaw after they forget to claim their cut and player1 and player4 forgot to claim their cut and don't know the flaw

  • Players = [player1, player2, player3, player4]

  • rewards = [400, 200, 200, 200]

  • totalRewards = 1000

  1. the owner call pot::closeContest and recieve his cut

  2. since there is no claimants yet the rest remain in the pot contract

  3. player2 and player3 call pot::claimCut and they recieve their cut and become claimants

  4. the owner call pot::closeContest again to recieve anotherCut

  5. player2 and player3 recieve more tokens since they become claimants

  • use the Following Poc in TestMyCut.t.sol

    • Don't forget to change player array to 4 players

​
function testUserCanClaimAfterClosingPotAndBecomeEligibleForMoreRewards()
public
mintAndApproveTokens
{
vm.startPrank(user);
rewards = [400, 200, 200, 200];
totalRewards = 1000;
contest = ContestManager(conMan).createContest(
players,
rewards,
IERC20(ERC20Mock(weth)),
totalRewards
);
ContestManager(conMan).fundContest(0);
vm.stopPrank();
​
//Checking Balances
uint256 claimantBalanceBefore2 = ERC20Mock(weth).balanceOf(player2);
assertEq(claimantBalanceBefore2, 0);
uint claimantBalanceBefore3 = ERC20Mock(weth).balanceOf(player3);
assertEq(claimantBalanceBefore3, 0);
assertEq(ERC20Mock(weth).balanceOf(conMan), 0);
​
//closing Contest
vm.warp(91 days);
vm.startPrank(user);
ContestManager(conMan).closeContest(contest);
vm.stopPrank();
assertEq(ERC20Mock(weth).balanceOf(address(conMan)), 100); // 10% of the remainingRewards(1000)
​
//Two Player found this flaw so they calimed after 90 days and pushed into claimants array
vm.prank(player2);
Pot(contest).claimCut();
vm.prank(player3);
Pot(contest).claimCut();
assertEq(ERC20Mock(weth).balanceOf(player2), 200);
assertEq(ERC20Mock(weth).balanceOf(player3), 200);
​
//The Owner close the contest again so he will get another ownerCut
//new remaning rewards = 1000-200-200=600 (200,200) are the player2 ,player3 cut they claimed
vm.startPrank(user);
ContestManager(conMan).closeContest(contest);
vm.stopPrank();
assertEq(ERC20Mock(weth).balanceOf(address(conMan)), 160); // 10% of the remaningRewards(600) + 10% of old remainingRewards(1000)
​
//Tracking Balances of player2 and player3 that become eligible to distrbuting of remmaining rewards
uint256 claimantBalanceAfter = ERC20Mock(weth).balanceOf(player2);
assertEq(claimantBalanceAfter, 335);//200 =>his cut + 135=>his cut from the new remaning rewards
uint claimantBalanceAfter3 = ERC20Mock(weth).balanceOf(player3);
assertEq(claimantBalanceAfter3, 335);//200 =>his cut + 135=>his cut from the new remaning rewards
}

Impact

  • Unfair Advanatage for users.the users who discovered this flow will claimCut any time

Tools Used

Manual Review + Foundry

Recommendations

  • you can add the following implementation in these functions pot::closeContes and pot::claimCut

  1. add a bool represent the contest status (closed or opened)

bool private isClosed;

  1. set the value of the flag to true at the end of pot::closeContest

​
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);
}
}
+ isClosed = true;
}
​

  1. add the following condition to pot::claimCut to prevent claiming after closing Contest

​
function claimCut() public {
address player = msg.sender;
uint256 reward = playersToRewards[player];
​
+ if(isClosed) {revert Pot__AlreadyClosed();}
​
if (reward <= 0) {
revert Pot__RewardNotFound();
}
playersToRewards[player] = 0;
remainingRewards -= reward;
claimants.push(player);
_transferReward(player, reward);
}
​
​
Updates

Lead Judging Commences

equious Lead Judge about 1 year ago
Submission Judgement Published
Invalidated
Reason: Other

Appeal created

FindingNem0 Submitter
about 1 year ago
equious Lead Judge
about 1 year ago
equious Lead Judge about 1 year ago
Submission Judgement Published
Invalidated
Reason: Other

Support

FAQs

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