Impact: H
Likelihood: H
Root + Impact
Description
-
Self transfer of ERC20 should not change balance of an account, but it does.
-
Also, this issue breaks these two invariants:
"no user balance should be greater than the token's total supply" - there is a test below that shows that the account's balance is greater than the token's total supply;
"sum of users' balances should not be greater than the token's total supply" - the same test below shows the opposite.
The root of the issue is in the ERC20Internals::_transfer function code - the balance of the receiver is recorded too early - see the code snippet below.
The issue occurs when self-transfer using ERC20::transfer and ERC20::transferFrom functions because both call ERC20Internals::_transfer function.
function _transfer(address from, address to, uint256 value) internal returns (bool success) {
assembly ("memory-safe") {
if iszero(from) {
mstore(0x00, shl(224, 0x96c6fd1e))
mstore(add(0x00, 4), 0x00)
revert(0x00, 0x24)
}
if iszero(to) {
mstore(0x00, shl(224, 0xec442f05))
mstore(add(0x00, 4), 0x00)
revert(0x00, 0x24)
}
let ptr := mload(0x40)
let baseSlot := _balances.slot
mstore(ptr, from)
mstore(add(ptr, 0x20), baseSlot)
let fromSlot := keccak256(ptr, 0x40)
let fromAmount := sload(fromSlot)
mstore(ptr, to)
mstore(add(ptr, 0x20), baseSlot)
@> let toSlot := keccak256(ptr, 0x40)
@> let toAmount := sload(toSlot)
if lt(fromAmount, value) {
mstore(0x00, shl(224, 0xe450d38c))
mstore(add(0x00, 4), from)
mstore(add(0x00, 0x24), fromAmount)
mstore(add(0x00, 0x44), value)
revert(0x00, 0x64)
}
sstore(fromSlot, sub(fromAmount, value))
sstore(toSlot, add(toAmount, value))
success := 1
mstore(ptr, value)
log3(ptr, 0x20, 0xddf252ad1be2c89b69c2b068fc378daa952ba7f163c4a11628f55a4df523b3ef, from, to)
}
}
Risk
Likelihood:
Impact:
Proof of Concept
Please add the following test to Token.t.sol:
function test_self_transfer_breaks_invariant(address account, uint256 amount, uint256 amountToTransfer) public {
vm.assume(account != address(0));
vm.assume(amount > 1);
uint256 adjustedAmount = bound(amount, 1, amount/2);
amountToTransfer = bound(amountToTransfer, 1, adjustedAmount);
token.mint(account, adjustedAmount);
uint256 balanceAccount1 = token.balanceOf(account);
assertEq(token.totalSupply(), adjustedAmount);
assertEq(balanceAccount1, adjustedAmount);
vm.prank(account);
vm.expectEmit(true, true, false, true);
emit Transfer(account, account, amountToTransfer);
token.transfer(account, amountToTransfer);
uint256 balanceAccount2 = token.balanceOf(account);
assertEq(token.totalSupply(), adjustedAmount);
assert(balanceAccount2 != balanceAccount1);
assert(balanceAccount2 != adjustedAmount);
assertGt(balanceAccount2, token.totalSupply());
}
Recommended Mitigation
Balance of the receiver should be recorded after the subtraction of the transferred amount from the sender.
function _transfer(address from, address to, uint256 value) internal returns (bool success) {
assembly ("memory-safe") {
if iszero(from) {
mstore(0x00, shl(224, 0x96c6fd1e))
mstore(add(0x00, 4), 0x00)
revert(0x00, 0x24)
}
if iszero(to) {
mstore(0x00, shl(224, 0xec442f05))
mstore(add(0x00, 4), 0x00)
revert(0x00, 0x24)
}
let ptr := mload(0x40)
let baseSlot := _balances.slot
mstore(ptr, from)
mstore(add(ptr, 0x20), baseSlot)
let fromSlot := keccak256(ptr, 0x40)
let fromAmount := sload(fromSlot)
mstore(ptr, to)
mstore(add(ptr, 0x20), baseSlot)
- let toSlot := keccak256(ptr, 0x40)
- let toAmount := sload(toSlot)
if lt(fromAmount, value) {
mstore(0x00, shl(224, 0xe450d38c))
mstore(add(0x00, 4), from)
mstore(add(0x00, 0x24), fromAmount)
mstore(add(0x00, 0x44), value)
revert(0x00, 0x64)
}
sstore(fromSlot, sub(fromAmount, value))
+ let toSlot := keccak256(ptr, 0x40)
+ let toAmount := sload(toSlot)
sstore(toSlot, add(toAmount, value))
success := 1
mstore(ptr, value)
log3(ptr, 0x20, 0xddf252ad1be2c89b69c2b068fc378daa952ba7f163c4a11628f55a4df523b3ef, from, to)
}
}
