MyCut
First Flight #23
Beginner FriendlyFoundry
100 EXP
View results
Previous Next
Submission Details
Severity: high
Invalid

Unchecked ERC20 Transfer Return Values

Hoover

Summary

The Pot.sol contract is vulnerable to failing transfers due to the unchecked return values of ERC20 token transfers in Pot::closePot and Pot:_transferReward. Specifically, the functions does not verify whether the token transfers to the manager and claimants are successful. This oversight can be exploited by using a custom ERC20 token that always returns true for transfers without actually transferring any tokens, thereby preventing the manager or claimers from receiving their cut.

Vulnerability Details

Affected code - https://github.com/Cyfrin/2024-08-MyCut/blob/946231db0fe717039429a11706717be568d03b54/src/Pot.sol#L55-L66

The vulnerability is located in the closePot and _transferReward function, where the return value of the transfer function is not checked:

function closePot() external onlyOwner {
...
if (remainingRewards > 0) {
...
@> i_token.transfer(msg.sender, managerCut);
...
for (uint256 i = 0; i < claimants.length; i++) {
_transferReward(claimants[i], claimantCut);
}
}
}
​
function _transferReward(address player, uint256 reward) internal {
@> i_token.transfer(player, reward);
}

The function attempts to transfer a portion of the remaining rewards to the manager. However, if the token used does not actually perform the transfer but still returns true, the manager will not receive their cut. This can be exploited by deploying a custom ERC20 token that simulates this behavior, effectively blocking the manager's transfer or claimants to receive their rewards.

Impact

The primary impact of this vulnerability is that it allows an attacker to prevent the manager from receiving their cut of the rewards or for claimants to receive their rewards. This could lead to financial losses for the manager and undermine the intended functionality of the contract.

Explotation

  1. Attacker creates an exploit token

  2. Attacker initiates a pot

  3. Owner tries to close down pot

PoC

function testExploitClosePot() public mintAndApproveTokens {
// Create contest with exploit token
vm.startPrank(user);
contest = ContestManager(conMan).createContest(players, rewards, IERC20(address(exploitToken)), totalRewards);
ContestManager(conMan).fundContest(0);
vm.stopPrank();
​
// Fast forward time to allow closing the pot
vm.warp(91 days);
console.log("Manager balance before: ", exploitToken.balanceOf(user));
​
​
// Close the contest
vm.startPrank(user);
ContestManager(conMan).closeContest(contest);
vm.stopPrank();
​
// Check manager balance (should be unchanged)
console.log("Manager balance after: ", exploitToken.balanceOf(user));
}
Manager balance before: 2000000000000000000000
Manager balance after: 2000000000000000000000

Exploit token contains this transfer function

function transfer(address to, uint256 amount) public override returns (bool) {
return true;
}

Tools Used

Manual Review

Recommendations

Always check the return value of ERC20 transfer and transferFrom functions. Revert the transaction if the transfer fails:

bool success = i_token.transfer(msg.sender, managerCut);
if (!success) {
revert Pot__TransferFailed();
}

Or use SafeERC20 which automatically handles return value checks and reverts on failure:

import "@openzeppelin/contracts/token/ERC20/utils/SafeERC20.sol";
​
using SafeERC20 for IERC20;
​
i_token.safeTransfer(msg.sender, managerCut);

By implementing these recommendations, you can mitigate the risk of transfer failures and ensure secure and reliable token transfers in your contract.

Updates

Lead Judging Commences

equious Lead Judge about 1 year ago
Submission Judgement Published
Invalidated
Reason: Known issue

Appeal created

equious Lead Judge about 1 year ago
Submission Judgement Published
Invalidated
Reason: Known issue

Support

FAQs

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