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

Unchecked ERC20 transfer return values let a failed transfer wipe a player's claim

ward0x

Root + Impact

Description

  • Token movements should either succeed or revert, so that the bookkeeping matches real balances.

  • fundContest, claimCut (via _transferReward) and closePot call transfer / transferFrom and ignore the returned bool. ERC20 tokens that signal failure by returning false instead of reverting are allowed by the standard; with such a token a failed transfer is treated as success.

@> token.transferFrom(msg.sender, address(pot), totalRewards); // ContestManager.fundContest
@> i_token.transfer(msg.sender, managerCut); // Pot.closePot
function _transferReward(address player, uint256 reward) internal {
@> i_token.transfer(player, reward); // Pot.claimCut / closePot
}

Risk

Likelihood:

  • Only with a token that returns false on failure; scope is limited to standard ERC20, and some standard tokens behave this way.

Impact:

  • In claimCut the player's allocation is zeroed and remainingRewards reduced even if nothing was sent, so the player loses the claim; a failed transferFrom leaves a Pot unfunded while looking funded.

Proof of Concept

The test uses a minimal ERC20 whose transfer returns false instead of reverting (allowed by the standard). The player calls claimCut while transfers fail: the call succeeds, the player receives nothing, their allocation is wiped, and any later claim reverts with Pot__RewardNotFound. The reward is lost to that player even though the tokens are still in the Pot.

function test_F6_falseReturnLosesClaim() public {
FalseToken t = new FalseToken(); // transfer/transferFrom return false when `fail` is set
vm.startPrank(owner);
ContestManager cm = new ContestManager();
t.mint(owner, 100 ether);
t.approve(address(cm), type(uint256).max);
address[] memory ps = new address[](1); ps[0] = p1;
uint256[] memory rs = new uint256[](1); rs[0] = 100 ether;
Pot pot = Pot(cm.createContest(ps, rs, IERC20(address(t)), 100 ether));
cm.fundContest(0);
vm.stopPrank();
​
t.setFail(true); // transfer now returns false
vm.prank(p1);
pot.claimCut(); // does not revert
assertEq(t.balanceOf(p1), 0); // nothing received
assertEq(pot.checkCut(p1), 0); // allocation already wiped
​
t.setFail(false);
vm.prank(p1);
vm.expectRevert(Pot.Pot__RewardNotFound.selector);
pot.claimCut(); // can never claim again
}

The same pattern in fundContest lets a failed transferFrom leave a Pot unfunded while the contest looks funded, and in closePot lets the manager / claimant payouts silently fail.

Recommended Mitigation

+import {SafeERC20} from "lib/openzeppelin-contracts/contracts/token/ERC20/utils/SafeERC20.sol";
+ using SafeERC20 for IERC20;
- i_token.transfer(player, reward);
+ i_token.safeTransfer(player, reward);

Apply the same to closePot and fundContest (safeTransferFrom). SafeERC20 reverts when a token returns false and also handles tokens that return nothing, so the bookkeeping (playersToRewards, remainingRewards) is only updated when the tokens really moved.

Updates

Lead Judging Commences

ai-first-flight-judge Lead Judge about 9 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!