Summary
Lack of return value handling for token transfer/transferFrom function could potentially affect any downstream process / UI which depends on the boolean status for proper rendering or processing
https://github.com/Cyfrin/2024-08-MyCut/blob/946231db0fe717039429a11706717be568d03b54/src/ContestManager.sol#L37
https://github.com/Cyfrin/2024-08-MyCut/blob/946231db0fe717039429a11706717be568d03b54/src/Pot.sol#L55
https://github.com/Cyfrin/2024-08-MyCut/blob/946231db0fe717039429a11706717be568d03b54/src/Pot.sol#L65
Vulnerability Details
In standard IERC20 implementation, there's a return boolean value for transfer and transferFrom function. Within the ContestManager and Pot contracts, it was found that the return value involving these functions were not handled properly with an event emitted upon successful transaction or revert with customized error if transaction fails.
In ContestManager 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);
}
In Pot contract :
function closePot() external onlyOwner {
if (block.timestamp - i_deployedAt < 90 days) {
revert Pot__StillOpenForClaim();
}
if (remainingRewards > 0) {
uint256 managerCut = remainingRewards / managerCutPercent;
<@@>! i_token.transfer(msg.sender, managerCut);
uint256 claimantCut = (remainingRewards - managerCut) / i_players.length;
for (uint256 i = 0; i < claimants.length; i++) {
_transferReward(claimants[i], claimantCut);
}
}
}
function _transferReward(address player, uint256 reward) internal {
<@@>! i_token.transfer(player, reward);
}
Impact
Any downstream process or UI that depends on the status of the transaction outcome of transfer/transferFrom could be affected and not able to process or render information properly.
Tools Used
Manual review
Recommendations
To include proper handling based on the transaction status as demonstrated below :
In ContestManager 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);
+ bool success = token.transferFrom(msg.sender, address(pot), totalRewards);
+ if (!success) {
+ revert ContestManager__fundContestFailed;
+ }
+ emit ContestManager__fundedContest;
}
In Pot contract :
function closePot() external onlyOwner {
if (block.timestamp - i_deployedAt < 90 days) {
revert Pot__StillOpenForClaim();
}
if (remainingRewards > 0) {
uint256 managerCut = remainingRewards / managerCutPercent;
- i_token.transfer(msg.sender, managerCut);
+ bool success = i_token.transfer(msg.sender, managerCut);
+ if (!success) {
+ revert Pot__transferManagerCutFailed;
+ };
+ emit Pot__transferedManagerCut;
uint256 claimantCut = (remainingRewards - managerCut) / i_players.length;
for (uint256 i = 0; i < claimants.length; i++) {
_transferReward(claimants[i], claimantCut);
}
}
}
function _transferReward(address player, uint256 reward) internal {
- i_token.transfer(player, reward);
+ bool success = i_token.transfer(player, reward);
+ if (!success) {
+ revert Pot__transferRewardFailed;
+ };
+ emit Pot__transferredReward;
}
