Root + Impact
Pot::closePot pays the manager and the claimants, but it does not change remainingRewards, and it does not mark the Pot as closed. So nothing stops the owner from calling closeContest for the same Pot again. If the Pot still holds tokens, the second call pays the manager and the claimants again with the same amounts.
Description
A close should happen only once. After it, the Pot should have nothing left to distribute. But closePot never reduces remainingRewards:
function closePot() external onlyOwner {
...
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);
}
}
}
remainingRewards stays the same after the first close, so the check remainingRewards > 0 is still true on the second call and all payments are repeated.
The second call only works if the Pot still holds tokens after the first close. This is the case in these situations:
If the Pot is empty after the first close, the second call fails because there are no tokens to send. That is not a protection by the code: it only works by accident.
Risk
Likelihood: Low
Only the owner can call closeContest, so the owner has to call it again for the same contest (for example, by mistake or a double click).
Nothing in the code warns or stops the owner.
The Pot must still hold tokens after the first close.
Impact: Medium
Example from the PoC: 3 players, only player1 claims. The first close pays the manager 5 and player1 15. The second close pays the manager 5 and player1 15 again.
So the manager gets 10 instead of 5, and player1 gets 15 more than the protocol intends.
This money comes out of the tokens that are still in the Pot. Players who did not claim yet lose part of that money.
Only the money of this one contest is affected.
Proof of Concept
Add the following functions inside the existing TestMyCut contract in test/TestMyCut.t.sol.
address player3 = makeAddr("player3");
function _createFundedPot(
uint256 r1,
uint256 r2,
uint256 r3,
uint256 total
) internal returns (Pot) {
address[] memory p = new address[](3);
p[0] = player1;
p[1] = player2;
p[2] = player3;
uint256[] memory r = new uint256[](3);
r[0] = r1;
r[1] = r2;
r[2] = r3;
vm.startPrank(user);
contest = ContestManager(conMan).createContest(
p,
r,
IERC20(ERC20Mock(weth)),
total
);
ContestManager(conMan).fundContest(0);
vm.stopPrank();
return Pot(contest);
}
function test_PoC_doubleClose_paysTwice() public mintAndApproveTokens {
Pot pot = _createFundedPot(10, 20, 30, 60);
vm.prank(player1);
pot.claimCut();
vm.warp(block.timestamp + 91 days);
uint256 remainingBeforeClose = pot.getRemainingRewards();
vm.prank(user);
ContestManager(conMan).closeContest(address(pot));
assertEq(weth.balanceOf(conMan), 5);
assertEq(weth.balanceOf(player1), 25);
assertEq(weth.balanceOf(address(pot)), 30);
uint256 remainingAfterFirstClose = pot.getRemainingRewards();
uint256 conManAfterFirstClose = weth.balanceOf(conMan);
uint256 player1AfterFirstClose = weth.balanceOf(player1);
vm.prank(user);
ContestManager(conMan).closeContest(address(pot));
console.log("Remaining before close:", remainingBeforeClose);
console.log(
"Remaining after first close", remainingAfterFirstClose);
console.log("Remaining after second close", pot.getRemainingRewards());
console.log("Manager paid again:", weth.balanceOf(conMan) - conManAfterFirstClose);
console.log("Player1 paid again:", weth.balanceOf(player1) - player1AfterFirstClose);
assertEq(remainingAfterFirstClose, remainingBeforeClose);
assertEq(pot.getRemainingRewards(), remainingAfterFirstClose);
assertGt(weth.balanceOf(conMan), conManAfterFirstClose);
assertGt(weth.balanceOf(player1), player1AfterFirstClose);
}
Run from the repository root:
forge test --mt test_PoC_doubleClose_paysTwice -vv
Output:
[PASS] test_PoC_doubleClose_paysTwice() (gas: 1228699)
Logs:
...
Remaining before close: 50
Remaining after first close 50
Remaining after second close 50
Manager paid again: 5
Player1 paid again: 15
Recommended Mitigation
Update the state before sending any tokens, so a second call has nothing left to pay:
function closePot() external onlyOwner {
...
if (remainingRewards > 0) {
uint256 managerCut = remainingRewards / managerCutPercent;
+ uint256 distributable = remainingRewards - managerCut;
+ remainingRewards = 0; // update state before the transfers
i_token.transfer(msg.sender, managerCut);
- uint256 claimantCut = (remainingRewards - managerCut) / i_players.length;
+ uint256 claimantCut = distributable / i_players.length;
...
}
....
}
