Summary
Duplicate Player Addresses Allowed in Contest Creation, causes unfair distribution
Vulnerability Details
https://github.com/Cyfrin/2024-08-MyCut/blob/main/src/Pot.sol#L32-L34
The Pot contract, when initialized through the ContestManager createContest function, does not perform a check for duplicate addresses in the players array.
This oversight allows the same address to be listed multiple times as a contestant, potentially leading to loss of funds that could have been allotted to unique address.
Here is how amount alloted is handled in Pot contract:
....
for (uint256 i = 0; i < i_players.length; i++) {
@> playersToRewards[i_players[i]] = i_rewards[i];
}
...
here we can see if an address is added multiple times, it's reward amount will be overridden by latest one. That will lock the previously alloted amount in the contract.
POC
Add a following test in existing test suite:
function testDuplicatePlayerAddresses() public mintAndApproveTokens {
address[] memory duplicatePlayers = new address[](3);
duplicatePlayers[0] = makeAddr("player1");
duplicatePlayers[1] = makeAddr("player2");
duplicatePlayers[2] = makeAddr("player1");
uint256[] memory duplicateRewards = new uint256[](3);
duplicateRewards[0] = 500;
duplicateRewards[1] = 200;
duplicateRewards[2] = 100;
vm.startPrank(user);
address duplicateContest = ContestManager(conMan).createContest(
duplicatePlayers,
duplicateRewards,
IERC20(ERC20Mock(weth)),
800
);
ContestManager(conMan).fundContest(0);
vm.stopPrank();
address[] memory contests = ContestManager(conMan).getContests();
assertEq(
contests[contests.length - 1],
duplicateContest,
"Contest with duplicate addresses should be created"
);
vm.startPrank(player1);
Pot(duplicateContest).claimCut();
uint256 claimAmount = ERC20Mock(weth).balanceOf(player1);
vm.stopPrank();
console.log(
claimAmount,
"claimed by user 1"
);
vm.warp(block.timestamp + 91 days);
vm.prank(user);
ContestManager(conMan).closeContest(duplicateContest);
uint256 fundStucked = ERC20Mock(weth).balanceOf(address(duplicateContest));
console.log("fundStucked:", fundStucked);
}
run forge test --mt testDuplicatePlayerAddresses -vv in the terminal, it will return following output:
[⠊] Compiling...
[⠑] Compiling 1 files with Solc 0.8.20
[⠘] Solc 0.8.20 finished in 1.68s
Compiler run successful!
Ran 1 test for test/TestMyCut.t.sol:TestMyCut
[PASS] testDuplicatePlayerAddresses() (gas: 887300)
Logs:
User Address: 0x6CA6d1e2D5347Bfab1d91e883F1915560e09129D
Contest Manager Address 1: 0x7BD1119CEC127eeCDBa5DCA7d1Bd59986f6d7353
Minting tokens to: 0x6CA6d1e2D5347Bfab1d91e883F1915560e09129D
Approved tokens to: 0x7BD1119CEC127eeCDBa5DCA7d1Bd59986f6d7353
100 Player1 should be able to claim twice
fundStucked: 420
Suite result: ok. 1 passed; 0 failed; 0 skipped; finished in 2.68ms (1.10ms CPU time)
Impact
Loss of funds / Unfair distribution
Tools Used
Manual Review
Recommendations
Add a check for duplicate address
constructor (.....) {
.... existing code
+ for (uint256 i = 0; i < players.length; i++) {
+ for (uint256 j = i + 1; j < players.length; j++) {
+ require(players[i] != players[j], "Duplicate player addresses are not allowed");
+ }
+ }
for (uint256 i = 0; i < i_players.length; i++) {
playersToRewards[i_players[i]] = i_rewards[i];
}
}
