Critical: Unchecked arithmetic allows balance overflow in transfers

[adamgall]

Description

All transfer and mint operations place recipient balance increments inside unchecked blocks without validating against overflow. This allows an attacker to overflow a recipient's balance, wrapping it to a small value.

Impact

• Severity: Critical
• Attack vector: Transfer tokens to an account with near-max balance → balance wraps to small number
• Enables theft of tokens through balance manipulation
• Breaks core ERC-20 invariant: sum of balances should equal totalSupply
• Can brick accounts by making their balances unusable

Proof of Concept

// Setup:
// Alice has: 100 tokens
// Bob has: type(uint256).max - 50 tokens

// Attack:
alice.transfer(bob, 100);

// Result:
// Bob's balance overflows and wraps to: 49
// Bob lost (max - 50) tokens!
// Alice successfully stole Bob's tokens

Vulnerable Locations

ERC20Facet.sol

1. transfer() (line 169):

unchecked {
    s.balanceOf[msg.sender] = fromBalance - _value;
    s.balanceOf[_to] += _value;  // ❌ Can overflow
}

2. transferFrom() (line 200):

unchecked {
    s.allowances[_from][msg.sender] = currentAllowance - _value;
    s.balanceOf[_from] = fromBalance - _value;
    s.balanceOf[_to] += _value;  // ❌ Can overflow
}

LibERC20.sol

1. mint() (lines 75-76):

unchecked {
    s.totalSupply += _value;  // ❌ Can overflow
    s.balanceOf[_account] += _value;  // ❌ Can overflow
}

2. transferFrom() (line 125):

unchecked {
    s.allowances[_from][msg.sender] = currentAllowance - _value;
    s.balanceOf[_from] = fromBalance - _value;
    s.balanceOf[_to] += _value;  // ❌ Can overflow
}

3. transfer() (line 145):

unchecked {
    s.balanceOf[msg.sender] = fromBalance - _value;
    s.balanceOf[_to] += _value;  // ❌ Can overflow
}

Why Subtractions Are Safe

The subtractions (sender balance, allowances) are protected by explicit checks:

• if (fromBalance < _value) ensures subtraction can't underflow
• if (currentAllowance < _value) ensures allowance subtraction is safe

Why Additions Are NOT Safe

Recipient balance increments have NO validation. The code assumes:

s.balanceOf[_to] + _value <= type(uint256).max

But this is not checked anywhere, making overflow possible.

Solution

Move recipient balance updates outside unchecked blocks to use Solidity 0.8+ overflow protection:

unchecked {
    s.balanceOf[msg.sender] = fromBalance - _value;  // Safe - checked above
}
s.balanceOf[_to] += _value;  // Safe - Solidity checks overflow

Note on Gas

The gas increase is negligible (~20 gas per transfer) and is standard for all secure ERC-20 implementations (OpenZeppelin, Solmate, etc.).