Root + Impact
Description
-
Normally, when a contract transfers tokens using transferFrom, it should either succeed or completely fail (revert). Some tokens like ZRX or old USDT don't follow this rule — they return false instead of reverting when something goes wrong.
-
The fundContest function calls transferFrom but never checks if it returned true or false. So if the transfer fails (for example, because the owner forgot to approve the contract), the transaction still succeeds and the contract thinks everything is fine. The Pot ends up with zero tokens, but nobody gets an error.
function fundContest(uint256 index) public onlyOwner {
Pot pot = Pot(contests[index]);
IERC20 token = pot.getToken();
uint256 totalRewards = contestToTotalRewards[address(pot)];
if (token.balanceOf(msg.sender) < totalRewards) {
revert ContestManager__InsufficientFunds();
}
token.transferFrom(msg.sender, address(pot), totalRewards);
}
Risk
Likelihood:High
The owner will call fundContest at some point — that's the whole point of the function.
Many popular tokens (ZRX, USDT, etc.) behave this way and are commonly used.
The owner might simply forget to call approve first (happens more often than you'd think).
Impact:Medium
The Pot contract stays empty even though the transaction succeeded.
Winners won't be able to claim rewards later — the contract will try to send tokens that aren't there.
Owner loses gas fees and thinks the contest is funded when it's not.
Proof of Concept
Here's a Foundry test that proves the vulnerability. It creates a contest, tries to fund it without approving the manager, and shows that the transaction succeeds but the Pot receives nothing.
pragma solidity ^0.8.20;
import "forge-std/Test.sol";
import "../src/ContestManager.sol";
import "../src/Pot.sol";
import "../src/ERC20Mock.sol";
contract ContestManagerTest is Test {
ContestManager manager;
ERC20Mock token;
address owner = address(0x123);
address user = address(0x456);
function setUp() public {
vm.startPrank(owner);
manager = new ContestManager();
token = new ERC20Mock();
vm.stopPrank();
}
function testUncheckedTransferFromVulnerability() public {
vm.startPrank(owner);
address[] memory players = new address[](1);
players[0] = user;
uint256[] memory rewards = new uint256[](1);
rewards[0] = 100;
address potAddress = manager.createContest(players, rewards, IERC20(address(token)), 100);
Pot pot = Pot(potAddress);
uint256 ownerBalanceBefore = token.balanceOf(owner);
manager.fundContest(0);
uint256 ownerBalanceAfter = token.balanceOf(owner);
assertEq(ownerBalanceAfter, ownerBalanceBefore, "Owner balance should not change");
uint256 potBalance = token.balanceOf(address(pot));
assertEq(potBalance, 0, "Pot should have zero balance because transfer failed");
vm.stopPrank();
}
}
Run it with forge test --match-test testUncheckedTransferFromVulnerability -vvv and it passes — proving the issue exists.
forge test --match-test testUncheckedTransferFromVulnerability -vvv
[⠢] Compiling...
[⠘] Compiling 1 files with Solc 0.8.33
[⠃] Solc 0.8.33 finished in 1.48s
Compiler run successful!
Ran 1 test for test/ContestManager.t.sol:ContestManagerTest
[PASS] testUncheckedTransferFromVulnerability() (gas: 842715)
Suite result: ok. 1 passed; 0 failed; 0 skipped; finished in 10.48ms (719.85µs CPU time)
Ran 1 test suite in 59.12ms (10.48ms CPU time): 1 tests passed, 0 failed, 0 skipped (1 total tests)
Recommended Mitigation
Use OpenZeppelin's SafeERC20 library. It handles all the weird token behavior and reverts properly when a transfer fails.
+ import {SafeERC20} from "lib/openzeppelin-contracts/contracts/token/ERC20/utils/SafeERC20.sol";
contract ContestManager is Ownable {
+ using SafeERC20 for IERC20;
// ... rest of the contract ...
function fundContest(uint256 index) public onlyOwner {
Pot pot = Pot(contests[index]);
IERC20 token = pot.getToken();
uint256 totalRewards = contestToTotalRewards[address(pot)];
- if (token.balanceOf(msg.sender) < totalRewards) {
- revert ContestManager__InsufficientFunds();
- }
- token.transferFrom(msg.sender, address(pot), totalRewards);
+ token.safeTransferFrom(msg.sender, address(pot), totalRewards);
}
}
