MyCut
AI First Flight #8
Beginner FriendlyFoundry
EXP
View results
Previous Next
Submission Details
Impact: high
Likelihood: low
Invalid

`Pot` and `ContestManager` ignore the return value of `transfer`/`transferFrom`, so a failed transfer silently erases a player's reward

summus

Root + Impact

Description

  • ERC20 transfer returns bool. Some tokens signal failure by returning false instead of reverting.

  • Pot and ContestManager never check the returned value. In claimCut the player's reward is zeroed before the transfer, so a transfer that returns false leaves the player with nothing and no way to claim again.

function claimCut() public {
...
@> playersToRewards[player] = 0;
remainingRewards -= reward;
claimants.push(player);
@> _transferReward(player, reward); // return value ignored
}
​
function _transferReward(address player, uint256 reward) internal {
@> i_token.transfer(player, reward);
}

The same pattern is used in closePot (i_token.transfer(msg.sender, managerCut)) and in ContestManager::fundContest (token.transferFrom(...)).

Risk

Likelihood:

  • When the reward token returns false on failure, and a player claims before the Pot is funded. createContest and fundContest are separate transactions, and nothing stops a claim in between.

  • The README limits compatibility to standard ERC20 tokens, which lowers the likelihood.

Impact:

  • The player's reward is set to zero, they receive nothing, and claimCut reverts with Pot__RewardNotFound on every later attempt.

  • An unnoticed false in fundContest leaves a Pot unfunded while the manager believes it is funded.

Proof of Concept

The token returns false on insufficient balance. player1 claims before funding and loses the reward permanently.

contract FalseReturnToken is ERC20 {
constructor() ERC20("False", "FALSE") {}
function mint(address to, uint256 amount) external { _mint(to, amount); }
function transfer(address to, uint256 value) public override returns (bool) {
if (balanceOf(msg.sender) < value) return false;
return super.transfer(to, value);
}
}
​
function testUncheckedTransferLosesReward() public {
FalseReturnToken token = new FalseReturnToken();
address[] memory players = new address[](1);
players[0] = player1;
uint256[] memory rewards = new uint256[](1);
rewards[0] = 100;
​
vm.prank(admin);
Pot pot = Pot(conMan.createContest(players, rewards, IERC20(address(token)), 100));
​
vm.prank(player1);
pot.claimCut(); // does not revert
​
assertEq(token.balanceOf(player1), 0);
assertEq(pot.checkCut(player1), 0);
​
token.mint(admin, 100);
vm.startPrank(admin);
token.approve(address(conMan), 100);
conMan.fundContest(0);
vm.stopPrank();
​
vm.prank(player1);
vm.expectRevert(Pot.Pot__RewardNotFound.selector);
pot.claimCut(); // reward lost forever
}

Recommended Mitigation

Use OpenZeppelin SafeERC20 everywhere.

+ import {SafeERC20} from "lib/openzeppelin-contracts/contracts/token/ERC20/utils/SafeERC20.sol";
contract Pot is Ownable(msg.sender) {
+ using SafeERC20 for IERC20;
...
- i_token.transfer(msg.sender, managerCut);
+ i_token.safeTransfer(msg.sender, managerCut);
...
function _transferReward(address player, uint256 reward) internal {
- i_token.transfer(player, reward);
+ i_token.safeTransfer(player, reward);
}
​
- token.transferFrom(msg.sender, address(pot), totalRewards);
+ token.safeTransferFrom(msg.sender, address(pot), totalRewards);
Updates

Lead Judging Commences

ai-first-flight-judge Lead Judge about 2 hours ago
Submission Judgement Published
Invalidated
Reason: Incorrect statement

Support

FAQs

Can't find an answer? Chat with us on Discord, Twitter or Linkedin.

Give us feedback!