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

`Pot` does not check the return value of ERC-20 `transfer`, so a claim can be marked as paid while no tokens are sent

theo_mc

Root + Impact

Pot::_transferReward calls i_token.transfer(...) and ignores the returned bool. The EIP-20 standard allows a token to return false instead of reverting when a transfer fails, and says that callers must handle it. If this happens in claimCut, the Pot has already set the player's reward to 0, reduced remainingRewards and added the player to claimants, but no tokens were sent. The player cannot claim again, so the reward is lost.

Description

claimCut updates the state first and then calls _transferReward, which does not check the result:

function claimCut() public {
address player = msg.sender;
uint256 reward = playersToRewards[player];
if (reward <= 0) {
revert Pot__RewardNotFound();
}
playersToRewards[player] = 0;
remainingRewards -= reward;
claimants.push(player);
_transferReward(player, reward);
}
​
function _transferReward(address player, uint256 reward) internal {
i_token.transfer(player, reward); // return value is ignored
}

If transfer returns false (for example a token that blocks some addresses or is paused, and returns false instead of reverting), claimCut does not revert. The player's reward is already set to 0, so the player cannot try again.

All token calls in the protocol ignore the returned bool:

In fundContest, a false result means the owner can believe the Pot is funded when no tokens arrived.

The README says "Standard ERC20 Tokens Only". A token that returns false on failure is still allowed by EIP-20, so the code must handle it.

Risk

Likelihood: Low

It needs a token that returns false instead of reverting when a transfer fails.
The token is chosen by the owner, and players cannot influence it.

Impact: Medium

The affected player loses the whole reward: the balance is unchanged, checkCut is 0 and the claim cannot be repeated.
The Pot keeps more tokens than remainingRewards says, so these tokens belong to nobody.
Only the money of this one contest is affected.

Proof of Concept

Add the following inside the existing TestMyCut contract in test/TestMyCut.t.sol. It uses the existing setup, imports and the mintAndApproveTokens modifier.

address player3 = makeAddr("player3");
​
function _createFundedPot(
uint256 r1,
uint256 r2,
uint256 r3,
uint256 total
) internal returns (Pot) {
address[] memory p = new address[](3);
p[0] = player1;
p[1] = player2;
p[2] = player3;
​
uint256[] memory r = new uint256[](3);
r[0] = r1;
r[1] = r2;
r[2] = r3;
​
vm.startPrank(user);
contest = ContestManager(conMan).createContest(
p,
r,
IERC20(ERC20Mock(weth)),
total
);
ContestManager(conMan).fundContest(0);
vm.stopPrank();
​
return Pot(contest);
}
​
function test_PoC_returnFalse_claimIsLost() public mintAndApproveTokens {
Pot pot = _createFundedPot(10, 20, 30, 60);
​
// the token returns false for this transfer instead of reverting
vm.mockCall(
address(weth),
abi.encodeWithSelector(IERC20.transfer.selector, player1, uint256(10)),
abi.encode(false)
);
​
vm.prank(player1);
pot.claimCut(); // does not revert
​
console.log("player1 balance :", weth.balanceOf(player1));
console.log("player1 reward left :", pot.checkCut(player1));
console.log("remainingRewards :", pot.getRemainingRewards());
console.log("tokens held by the Pot:", weth.balanceOf(address(pot)));
​
assertEq(weth.balanceOf(player1), 0); // no tokens received
assertEq(pot.checkCut(player1), 0); // reward is gone, claim cannot be repeated
assertGt(weth.balanceOf(address(pot)), pot.getRemainingRewards()); // 60 > 50
}

Run from the repository root:

forge test --mt test_PoC_returnFalse_claimIsLost -vv

Output:

[PASS] test_PoC_returnFalse_claimIsLost() (gas: 1146371)
Logs:
...
player1 balance : 0
player1 reward left : 0
remainingRewards : 50
tokens held by the Pot: 60

vm.mockCall makes transfer(player1, 10) return false, the same as a token that returns false for a failed transfer.

Recommended Mitigation

Use OpenZeppelin's SafeERC20 for every token call. It reverts when a transfer fails or returns false, so the whole claimCut reverts and the player's reward stays claimable:

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

The PoC covers claimCut. The other calls listed in the Description ignore the returned value in the same way and need the same fix.

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!