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

Unchecked ERC20 transfer / transferFrom return values

sharonkosgei4

Root + Impact

Description

  • Three ERC20 calls do not check their boolean return values. Non-standard tokens such as USDT return false on failure rather than reverting. Ignoring the return value means a failed transfer silently passes, leaving the contract in an inconsistent state.

// ContestManager.sol#37
token.transferFrom(msg.sender, address(pot), totalRewards); // return value ignored
​
// Pot.sol#55
i_token.transfer(msg.sender, managerCut); // return value ignored
​
// Pot.sol#65
i_token.transfer(player, reward);

Risk

Likelihood:

  • Non-reverting ERC20 tokens (e.g., USDT on Ethereum mainnet, older BEP-20 tokens, and various custom tokens) are widely deployed and commonly used as prize tokens in contest platforms.

  • While the standard OpenZeppelin ERC20 used in the test suite reverts on failure, the protocol places no restriction on which ERC20 can be used in a Pot.

  • Any non-reverting token triggers this vulnerability, and the impact ranges from a silently unfunded Pot to permanently lost claimant rewards.

Impact:

  • fundContest: Pot appears funded but holds no tokens; all subsequent claims silently fail.

  • closePot / _transferReward: manager or claimant receives nothing while remainingRewards is decremented, causing permanent fund loss.

Proof of Concept

The following test demonstrates that fundContest silently "succeeds" with a non-reverting token, leaving the Pot with zero balance, and that a subsequent claimCut zeroes the player's entitlement without delivering any tokens permanently destroying their reward with no recourse. Add it to test/PocTests.t.sol alongside the ReturnFalseERC20 helper contract and run forge test --match-test testM1 -vvv:

/// @dev Minimal ERC20 that always returns false from transfer/transferFrom without reverting.
contract ReturnFalseERC20 {
string public name = "FAIL";
string public symbol = "FAIL";
uint8 public decimals = 18;
mapping(address => uint256) public balanceOf;
mapping(address => mapping(address => uint256)) public allowance;
​
function mint(address to, uint256 amount) external { balanceOf[to] += amount; }
function approve(address spender, uint256 amount) external returns (bool) {
allowance[msg.sender][spender] = amount;
return true;
}
function transfer(address, uint256) external pure returns (bool) { return false; }
function transferFrom(address, address, uint256) external pure returns (bool) { return false; }
}
​
function testM1_UncheckedTransferReturnValue() public {
ReturnFalseERC20 badToken = new ReturnFalseERC20();
badToken.mint(owner, TOTAL_REWARDS);
​
address[] memory players = new address[](1);
players[0] = player1;
uint256[] memory rewards = new uint256[](1);
rewards[0] = TOTAL_REWARDS;
​
vm.startPrank(owner);
badToken.approve(address(conMan), TOTAL_REWARDS);
address contestAddr = conMan.createContest(
players, rewards, IERC20(address(badToken)), TOTAL_REWARDS
);
​
// fundContest does not revert even though transferFrom returns false
conMan.fundContest(0);
vm.stopPrank();
​
// BUG: Pot holds 0 tokens despite fundContest "succeeding"
assertEq(badToken.balanceOf(contestAddr), 0, "pot holds no tokens");
assertEq(badToken.balanceOf(owner), TOTAL_REWARDS, "owner still holds all tokens");
​
// claimCut also silently "succeeds" but player receives nothing
// and their entitlement is permanently zeroed before the transfer is attempted
vm.prank(player1);
Pot(contestAddr).claimCut();
​
assertEq(badToken.balanceOf(player1), 0, "player1 received nothing");
assertEq(Pot(contestAddr).checkCut(player1), 0, "entitlement permanently zeroed");
​
// Player1 can no longer retry — their reward mapping was already cleared
vm.prank(player1);
vm.expectRevert(Pot.Pot__RewardNotFound.selector);
Pot(contestAddr).claimCut();
​
console.log("--- M1 Results ---");
console.log("Pot token balance (should be TOTAL_REWARDS):", badToken.balanceOf(contestAddr));
console.log("Player1 balance (should be TOTAL_REWARDS):", badToken.balanceOf(player1));
}

Recommended Mitigation

Use OpenZeppelin's SafeERC20 library and apply safeTransfer / safeTransferFrom to all three call sites:

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

Lead Judging Commences

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