When branding a reward in Pot::claimCut, the user may not receive the reward.
Description
When the user calls the function to receive their share of the reward, there is a possibility that they will not receive anything. Because the _transferReward function is called, which does not check whether the transfer was successful.
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);
}
Risk
If for some reason the transaction fails and there is no verification that this is a failure. The mapping will be reduced by the entire amount for the user, but he will not receive money. Because the token did not return a revert, but only a false one.
Impact:
Proof of Concept
We create a token that only returns false on unsuccessful transactions.
We create a new Pot and do not fund it.
After that, Alice calls the function to get her reward, but her balance will be zero. And the mapping itself has also changed, although she did not receive anything.
pragma solidity ^0.8.20;
import {Test, console} from "forge-std/Test.sol";
import {Pot} from "../src/Pot.sol";
import {IERC20} from "@openzeppelin/contracts/token/ERC20/IERC20.sol";
contract MaliciousToken {
mapping(address => uint256) public balanceOf;
constructor() {
balanceOf[msg.sender] = 1000000 * 1e18;
}
function transfer(address to, uint256 amount) external returns (bool) {
if (balanceOf[msg.sender] < amount) {
return false;
}
balanceOf[msg.sender] -= amount;
balanceOf[to] += amount;
return true;
}
function approve(address spender, uint256 amount) external returns (bool) {
return true;
}
function transferFrom(
address from,
address to,
uint256 amount
) external returns (bool) {
if (balanceOf[from] < amount) {
return false;
}
balanceOf[from] -= amount;
balanceOf[to] += amount;
return true;
}
}
contract MyCut is Test {
MaliciousToken public token;
Pot public pot;
address public alice = makeAddr("alice");
address public bob = makeAddr("bob");
function setUp() public {
token = new MaliciousToken();
address[] memory players = new address[](2);
players[0] = alice;
players[1] = bob;
uint256[] memory rewards = new uint256[](2);
rewards[0] = 100 * 1e18;
rewards[1] = 100 * 1e18;
pot = new Pot(players, rewards, IERC20(address(token)), 200 * 1e18);
console.log("Setup complete");
console.log(
"Token balance of this contract: ",
token.balanceOf(address(this))
);
console.log("Pot contract address: ", address(pot));
}
function testSilentTransferFailed() public {
vm.prank(alice);
pot.claimCut();
assertEq(pot.checkCut(alice), 0);
assertEq(pot.getRemainingRewards(), 100 * 1e18);
assertEq(token.balanceOf(alice), 0);
}
}
Recommended Mitigation
The easiest way is to use secure transfers from OpenZeppelin.
-
Import the contract.
-
Replace the transfer function, which will return a rollback if the transaction fails.
+ import {SafeERC20} from "@openzeppelin/contracts/token/ERC20/utils/SafeERC20.sol";
function _transferReward(address player, uint256 reward) internal {
- i_token.transfer(player, reward);
+ i.token.safeTransfer(player, reward);
}
