MyCut
AI First Flight #8
Beginner FriendlyFoundry
EXP
View results
Previous Next
Submission Details
Impact: high
Likelihood: high
Invalid

`Pot::closePot()` may be called several times causing inflation of pot balance and players balance

valya

Impact: High
Likelihood: High

Root + Impact

Description

  • When the contest is considered as done, the owner calls Pot::closePot() and the rewards should be distributed among claimants and a contest manager, and it is logically correct that the remainingRewards should be assigned to zero, but this never happens.

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:

  • The issue occurs because remainingRewards value is not zeroed when Pot::closePot() is called.

Impact:

  • It it possible to call Pot::closePot() several times and the same amount of remainingRewards is used to calculate managerCut and claimantCut, so a manager and claimants receive extra funds and this may be done till there is enough tokens in the Pot.*

  • Also, it is possible to call Pot::claimCut() after Pot::closePot() and receive a reward back because remainingRewards is not zeroed.

Proof of Concept

Please, add the following test test_closeContestSeveralTimes to TestMyCut.t.sol:

address player3 = makeAddr("player3");
address player4 = makeAddr("player4");
address[] playersLocal = [player1, player2, player3, player4];
​
function test_closeContestSeveralTimes_callClaimAfterCloseContest() public mintAndApproveTokens {
vm.startPrank(user);
rewards = [500, 500, 500, 500];
totalRewards = 2000;
contest = ContestManager(conMan).createContest(playersLocal, rewards, IERC20(ERC20Mock(weth)), totalRewards);
ContestManager(conMan).fundContest(0);
vm.stopPrank();
// one player claimed
vm.startPrank(player1);
Pot(contest).claimCut(); // 3 rewards are left
vm.stopPrank();
vm.warp(91 days);
​
// close contest first time
uint256 conManBalanceBeforeCloseContest1 = ERC20Mock(weth).balanceOf(conMan);
uint256 playerBalanceBeforeCloseContest1 = ERC20Mock(weth).balanceOf(player1);
vm.prank(user);
ContestManager(conMan).closeContest(contest);
uint256 conManBalanceAfterCloseContest1 = ERC20Mock(weth).balanceOf(conMan);
uint256 playerBalanceAfterCloseContest1 = ERC20Mock(weth).balanceOf(player1);
uint256 managerCut1 = conManBalanceAfterCloseContest1 - conManBalanceBeforeCloseContest1;
console.log("managerCut1 ", managerCut1.toString());
​
uint256 playerProfit1 = playerBalanceAfterCloseContest1 - playerBalanceBeforeCloseContest1;
console.log("playerProfit1 ", playerProfit1.toString());
​
// close contest second time
uint256 conManBalanceBeforeCloseContest2 = ERC20Mock(weth).balanceOf(conMan);
uint256 playerBalanceBeforeCloseContest2 = ERC20Mock(weth).balanceOf(player1);
vm.prank(user);
ContestManager(conMan).closeContest(contest);
uint256 comManBalanceAfterCloseContest2 = ERC20Mock(weth).balanceOf(conMan);
uint256 playerBalanceAfterCloseContest2 = ERC20Mock(weth).balanceOf(player1);
​
uint256 managerCut2 = comManBalanceAfterCloseContest2 - conManBalanceBeforeCloseContest2;
console.log("managerCut2 ", managerCut2.toString());
​
uint256 playerProfit2 = playerBalanceAfterCloseContest2 - playerBalanceBeforeCloseContest2;
console.log("playerProfit2 ", playerProfit2.toString());
​
assertLt(managerCut1, managerCut1 + managerCut2);
assertLt(playerProfit1, playerProfit1 + playerProfit2);
​
// second player claimed
uint256 player2BalanceBeforeClaim = ERC20Mock(weth).balanceOf(player2);
vm.prank(player2);
Pot(contest).claimCut();
uint256 player2BalanceAfterClaim = ERC20Mock(weth).balanceOf(player2);
uint256 player2ProfitAfterClaim = player2BalanceAfterClaim - player2BalanceBeforeClaim;
console.log("player2ProfitAfterClaim ", player2ProfitAfterClaim.toString());
assertGt(player2ProfitAfterClaim, 0);
}

The logs show the actual doubled profit for a manager and a player after 2 consequent closings:

managerCut1 150
playerProfit1 337
managerCut2 150
playerProfit2 337
player2ProfitAfterClaim 500

Recommended Mitigation

The remainingRewards value should be assigned to zero once a contest is closed. Follow check-effect-interact pattern:

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

Lead Judging Commences

ai-first-flight-judge Lead Judge about 2 hours ago
Submission Judgement Published
Invalidated
Reason: Incorrect statement

Support

FAQs

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

Give us feedback!