[H-02] Incorrect distribution of rewards in Pot::closePot
Description: Pot::closePot allows contest manager to distribute the reward pool equally among the claimants. According to the protocol the remaining rewards left in the pool should be distributed equally to the players who claim within the 90 day period. This means that the after the manager's cut the remaining reward should be divided by the number of claimants claimants.length. But in the code it is divided by the number of players i_players.length. The vulnerable code is at https://github.com/Cyfrin/2024-08-MyCut/blob/main/src/Pot.sol#L57.
This leads to incorrect distribution of rewards to the claimants and leaves reward tokens in the pot. This leads to contest manager funds being lost.
Vulnerabilitiy Analysis: Imagine three players with addresses [a1, a2, a3] and corresponding rewards [25, 45, 30] are created into a pool. totalRewards is 100. Let's consider the below scenario in which players claim their rewards.
Player 2 claims their reward. This transfers 45 tokens to player 2 and remainingRewards is now set to 55
Player 1 claims their reward. This transfers 25 tokens to player 1 and remainingRewards is now set to 30
Let's assume 90 days have passed. player 3 has not claimed their reward. Contest manager has closed the pot by calling ContestManager::closeContest. Since the remaining reward tokens are 30, After deducting the managers cut, 27 tokens are to be distributed equally among the two claimants. i.e player 1, player 2. In the closeContest code 27 tokens is being divided by the number of players i_players.length in the pot. Instead it should be divided by number of claimants claimants.length.
Impact: Claimants wont receive the rewards correctly and will lead to funds being stuck in the pot forever.
Proof of code:
Add the below function to test/TestMyCut.t.sol
function testClaimsDistribution() public mintAndApproveTokens {
ContestManager cm = ContestManager(conMan);
uint playersLength = 3;
address[] memory p = new address[](playersLength);
uint256[] memory r = new uint256[](playersLength);
uint tr = 100;
p[0] = makeAddr("_player1");
p[1] = makeAddr("_player2");
p[2] = makeAddr("_player3");
r[0] = 25;
r[1] = 45;
r[2] = 30;
vm.startPrank(user);
address pot = cm.createContest(p, r, weth, tr);
cm.fundContest(0);
vm.stopPrank();
console.log(
"player1 token balance before claim: ",
weth.balanceOf(p[0])
);
console.log(
"player2 token balance before claim: ",
weth.balanceOf(p[1])
);
console.log(
"player3 token balance before claim: ",
weth.balanceOf(p[2])
);
console.log(
"token balance in pot after closing pot: ",
weth.balanceOf(pot)
);
console.log("\n\n");
vm.prank(p[1]);
Pot(pot).claimCut();
console.log(
"player1 token balance after player 2 claim: ",
weth.balanceOf(p[0])
);
console.log(
"player2 token balance after player 2 claim: ",
weth.balanceOf(p[1])
);
console.log(
"player3 token balance after player 2 claim: ",
weth.balanceOf(p[2])
);
console.log(
"token balance in pot after closing pot: ",
weth.balanceOf(pot)
);
console.log("\n\n");
vm.prank(p[0]);
Pot(pot).claimCut();
console.log(
"player1 token balance after player 1 claim: ",
weth.balanceOf(p[0])
);
console.log(
"player2 token balance after player 1 claim: ",
weth.balanceOf(p[1])
);
console.log(
"player3 token balance after player 1 claim: ",
weth.balanceOf(p[2])
);
console.log(
"token balance in pot after closing pot: ",
weth.balanceOf(pot)
);
console.log("\n\n");
vm.prank(user);
vm.warp(block.timestamp + 90 days + 1);
cm.closeContest(pot);
console.log(
"player1 token balance after claim: ",
weth.balanceOf(p[0])
);
console.log(
"player2 token balance after claim: ",
weth.balanceOf(p[1])
);
console.log(
"player3 token balance after claim: ",
weth.balanceOf(p[2])
);
console.log(
"\n\ntoken balance in pot after closing pot: ",
weth.balanceOf(pot)
);
assert(weth.balanceOf(pot) != 0);
}
Run the below test command in terminal
forge test --mt testClaimsDistribution -vv
Which leads to below output
[⠑] Compiling...
[⠆] Compiling 1 files with 0.8.20
[⠰] Solc 0.8.20 finished in 2.50s
Compiler run successful!
Ran 1 test for test/TestMyCut.t.sol:TestMyCut
[PASS] testClaimsDistribution() (gas: 969537)
Logs:
player1 token balance before claim: 0
player2 token balance before claim: 0
player3 token balance before claim: 0
token balance in pot after closing pot: 100
player1 token balance after player 2 claim: 0
player2 token balance after player 2 claim: 45
player3 token balance after player 2 claim: 0
token balance in pot after closing pot: 55
player1 token balance after player 1 claim: 25
player2 token balance after player 1 claim: 45
player3 token balance after player 1 claim: 0
token balance in pot after closing pot: 30
player1 token balance after claim: 34
player2 token balance after claim: 54
player3 token balance after claim: 0
token balance in pot after closing pot: 9
Suite result: ok. 1 passed; 0 failed; 0 skipped; finished in 2.01ms (842.50µs CPU time)
Ran 1 test suite in 241.92ms (2.01ms CPU time): 1 tests passed, 0 failed, 0 skipped (1 total tests)
As it is shown in the above output, The rewards present in the pot after players have claimed is 30. When the contest manager closes the pot, 10 percent of 30 tokens which is 3 tokens are sent to their account. The remaining 27 should be distributed to claimants. This should be done by dividing 27 tokens by 2. Instead it is being divided by 3 which is the number of players which results in claimants not receiving the reward and tokens getting stuck in the pot.
Impact: This could lead to reward tokens getting stuck in the protocol forever.
Recommended Mitigation: This could easily be avoided by dividing the remaining rewards by the number of claimants.
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 claimantCut = (remainingRewards - managerCut) /
+ claimants.length;
for (uint256 i = 0; i < claimants.length; i++) {
_transferReward(claimants[i], claimantCut);
}
}
}
Tools used: Foundry, VSCode.
