MyCut

Summary

ContestManager::createContest deploy the pot contract hence the owner of the pot is the ContestManager contract which can't handle the ownerCut

https://github.com/Cyfrin/2024-08-MyCut/blob/946231db0fe717039429a11706717be568d03b54/src/Pot.sol#L55

Vulnerability Details

When ContestManager::createContest deploy the pot contract the owner of the pot is the ContestManager contract.so when the 90 days pass and there is unclaimed rewards and the owner call pot::closeContest to get the ownerCut the ownerCut transfered to the ContestManager instead of the owne of the contestManger.since the contestManager has no withdraw mechanism the ownerCut will be locked forever.

• Use the following POC in TestMyCut.t.sol

  function testOwnerCutWillBeStuck() public mintAndApproveTokens {

  vm.startPrank(user);

  rewards = [500, 500];

  totalRewards = 1000;

  contest = ContestManager(conMan).createContest(

  players,

  rewards,

  IERC20(ERC20Mock(weth)),

  totalRewards

  );

  ContestManager(conMan).fundContest(0);

  vm.stopPrank();

  ​

  //Tracking user Balance Before

  uint userBalanceBefore=ERC20Mock(weth).balanceOf(user);

  console.log("User Balance Before: ", userBalanceBefore);

  ​

  vm.startPrank(player1);

  Pot(contest).claimCut();

  vm.stopPrank();

  ​

  //Tracking claimant Balance Before

  uint256 claimantBalanceBefore = ERC20Mock(weth).balanceOf(player1);

  console.log("Claimant Balance Before: ", claimantBalanceBefore);//500 from his cut

  ​

  vm.warp(91 days);

  ​

  //check contestManager Balance before

  assertEq(ERC20Mock(weth).balanceOf(address(conMan)), 0);

  ​

  //closing contest

  vm.startPrank(user);

  ContestManager(conMan).closeContest(contest);

  vm.stopPrank();

  ​

  //Tracking user Balance After

  uint userBalanceAfter=ERC20Mock(weth).balanceOf(user);

  console.log("User Balance After: ", userBalanceAfter);

  ​

  //Tracking claimant Balance After

  uint256 claimantBalanceAfter = ERC20Mock(weth).balanceOf(player1);

  console.log("Claimant Balance After: ", claimantBalanceAfter);

  ​

  //Proving that there is no change in userBalance after calling closeContest

  assertEq(userBalanceBefore, userBalanceAfter);

  //Proving that the OwnerCut is transfered to the ContestManager contract (this will be stuck )

  assertEq(ERC20Mock(weth).balanceOf(address(conMan)), 50);// remainingRewards/percentage = 500/10=50

  ​

  // proving that contest owner is the ContestManager contract

  assertEq(Pot(contest).owner(), address(conMan));

  }

Impact

Losing of funds aka the ownerCut

Tools Used

Manual Review + Foundry

Recommendations

• Use the following steps to implement this recommendation in pot::closeContest

1. import contestManager in pot contract

1. Pass the contestManager contract address in the constructor

1. add the contestManager address in contestManager::createContest