Description:
The Pot constructor initializes the playersToRewards mapping without checking for duplicate addresses in the players array. While this does not allow for multiple claims due to the Pot::ClaimCut function resetting rewards after a claim, it can still lead to incorrect initialization.
Impact:
Although duplicates do not lead to multiple reward claims, they can cause confusion and incorrect data in the initial setup. This could potentially mislead contract owners or users about the state of the rewards.
Proof of Concept:
Deploy the Pot contract with a players array that contains duplicates.
Observe that an duplicate address is initialized with the rewards for both instances in the array, but it can only claim once.
Add this to the TestMyCut.t.sol file.
This are the storage variables that have been used.
address duplicate1 = makeAddr("duplicate1");
address duplicate2 = makeAddr("duplicate1");
address[] duplicators = [duplicate1, duplicate2];
Then run this test function:
unction testCanCreateDuplicatePlayersinAFundedcontest() public mintAndApproveTokens {
console.log("This is the address of duplicate1::", duplicate1);
console.log("This is the address of duplicate2::", duplicate2);
assertEq(duplicate1, duplicate2);
vm.startPrank(user);
contest = ContestManager(conMan).createContest(duplicators, rewards, IERC20(ERC20Mock(weth)), 4);
ContestManager(conMan).fundContest(0);
vm.stopPrank();
vm.startPrank(duplicate1);
Pot(contest).claimCut();
vm.stopPrank();
vm.startPrank(duplicate2);
vm.expectRevert();
Pot(contest).claimCut();
vm.stopPrank();
}
These are the logs that were outputted:
Traces:
[834406] TestMyCut::testCanCreateDuplicatePlayersinAFundedcontest()
├─ [0] console::log("Minting tokens to: ", user: [0x6CA6d1e2D5347Bfab1d91e883F1915560e09129D]) [staticcall]
│ └─ ← [Stop]
├─ [0] VM::startPrank(user: [0x6CA6d1e2D5347Bfab1d91e883F1915560e09129D])
│ └─ ← [Return]
├─ [29712] ERC20Mock::mint(user: [0x6CA6d1e2D5347Bfab1d91e883F1915560e09129D], 1000000000000000000000 [1e21])
│ ├─ emit Transfer(from: 0x0000000000000000000000000000000000000000, to: user: [0x6CA6d1e2D5347Bfab1d91e883F1915560e09129D], value: 1000000000000000000000 [1e21])
│ └─ ← [Stop]
├─ [24739] ERC20Mock::approve(ContestManager: [0x7BD1119CEC127eeCDBa5DCA7d1Bd59986f6d7353], 1000000000000000000000 [1e21])
│ ├─ emit Approval(owner: user: [0x6CA6d1e2D5347Bfab1d91e883F1915560e09129D], spender: ContestManager: [0x7BD1119CEC127eeCDBa5DCA7d1Bd59986f6d7353], value: 1000000000000000000000 [1e21])
│ └─ ← [Return] true
├─ [0] console::log("Approved tokens to: ", ContestManager: [0x7BD1119CEC127eeCDBa5DCA7d1Bd59986f6d7353]) [staticcall]
│ └─ ← [Stop]
├─ [0] VM::stopPrank()
│ └─ ← [Return]
├─ [0] console::log("This is the address of duplicate1::", duplicate1: [0x729108660e7835278C57075d9a5050e6671c4d90]) [staticcall]
│ └─ ← [Stop]
├─ [0] console::log("This is the address of duplicate2::", duplicate1: [0x729108660e7835278C57075d9a5050e6671c4d90]) [staticcall]
│ └─ ← [Stop]
├─ [0] VM::assertEq(duplicate1: [0x729108660e7835278C57075d9a5050e6671c4d90], duplicate1: [0x729108660e7835278C57075d9a5050e6671c4d90]) [staticcall]
│ └─ ← [Return]
├─ [0] VM::startPrank(user: [0x6CA6d1e2D5347Bfab1d91e883F1915560e09129D])
│ └─ ← [Return]
├─ [612006] ContestManager::createContest([0x729108660e7835278C57075d9a5050e6671c4d90, 0x729108660e7835278C57075d9a5050e6671c4d90], [3, 1], ERC20Mock: [0x5929B14F2984bBE5309c2eC9E7819060C31c970f], 4)
│ ├─ [508191] → new Pot@0x43e82d2718cA9eEF545A591dfbfD2035CD3eF9c0
│ │ ├─ emit OwnershipTransferred(previousOwner: 0x0000000000000000000000000000000000000000, newOwner: ContestManager: [0x7BD1119CEC127eeCDBa5DCA7d1Bd59986f6d7353])
│ │ └─ ← [Return] 1513 bytes of code
│ └─ ← [Return] Pot: [0x43e82d2718cA9eEF545A591dfbfD2035CD3eF9c0]
├─ [29108] ContestManager::fundContest(0)
│ ├─ [192] Pot::getToken() [staticcall]
│ │ └─ ← [Return] ERC20Mock: [0x5929B14F2984bBE5309c2eC9E7819060C31c970f]
│ ├─ [641] ERC20Mock::balanceOf(user: [0x6CA6d1e2D5347Bfab1d91e883F1915560e09129D]) [staticcall]
│ │ └─ ← [Return] 1000000000000000000000 [1e21]
│ ├─ [26084] ERC20Mock::transferFrom(user: [0x6CA6d1e2D5347Bfab1d91e883F1915560e09129D], Pot: [0x43e82d2718cA9eEF545A591dfbfD2035CD3eF9c0], 4)
│ │ ├─ emit Transfer(from: user: [0x6CA6d1e2D5347Bfab1d91e883F1915560e09129D], to: Pot: [0x43e82d2718cA9eEF545A591dfbfD2035CD3eF9c0], value: 4)
│ │ └─ ← [Return] true
│ └─ ← [Stop]
├─ [0] VM::stopPrank()
│ └─ ← [Return]
├─ [0] VM::startPrank(duplicate1: [0x729108660e7835278C57075d9a5050e6671c4d90])
│ └─ ← [Return]
├─ [70849] Pot::claimCut()
│ ├─ [25188] ERC20Mock::transfer(duplicate1: [0x729108660e7835278C57075d9a5050e6671c4d90], 1)
│ │ ├─ emit Transfer(from: Pot: [0x43e82d2718cA9eEF545A591dfbfD2035CD3eF9c0], to: duplicate1: [0x729108660e7835278C57075d9a5050e6671c4d90], value: 1)
│ │ └─ ← [Return] true
│ └─ ← [Stop]
├─ [0] VM::stopPrank()
│ └─ ← [Return]
├─ [0] VM::startPrank(duplicate1: [0x729108660e7835278C57075d9a5050e6671c4d90])
│ └─ ← [Return]
├─ [0] VM::expectRevert(custom error f4844814:)
│ └─ ← [Return]
├─ [440] Pot::claimCut()
│ └─ ← [Revert] Pot__RewardNotFound()
├─ [0] VM::stopPrank()
│ └─ ← [Return]
└─ ← [Stop]
From the logs above, we can see that the 2nd time the duplicate address tried to use the Pot::ClaimCut function it reverted as you can tell by the output of the custom error(Pot_RewardNotFound()).
Recommended Mitigation:
Implement a check in the constructor to ensure that each address in the Pot::players array is unique. This can be done by adding the addresses to a mapping and reverting if a duplicate is found.
The following mitigation can be implemented in the Pot codebase as seen below..
+mapping(address => bool) private isPlayerAdded;
constructor(address[] memory players, uint256[] memory rewards, IERC20 token, uint256 totalRewards) {
i_players = players;
i_rewards = rewards;
i_token = token;
i_totalRewards = totalRewards;
remainingRewards = totalRewards;
i_deployedAt = block.timestamp;
for (uint256 i = 0; i < i_players.length; i++) {
+ if (isPlayerAdded[i_players[i]]) {
+ revert Pot__DuplicatePlayer();
+ }
+ isPlayerAdded[i_players[i]] = true;
playersToRewards[i_players[i]] = i_rewards[i];
}
}
Explanation:
Duplicate Check: A mapping called isPlayerAdded has been introduced. This mapping tracks whether a player has already been added to the Pot::i_players array.
Constructor Update: In the constructor, when players are added, the contract checks if they are already present in the isPlayerAdded mapping. If a duplicate is found, the transaction is reverted with the custom error Pot__DuplicatePlayer.
Claim and Close Functions: The logic in Pot::claimCut and Pot::closePot remains unchanged, as this duplicate check ensures that the i_players array is unique from the beginning.
