`BalanceOf` check vulnerability allows over-transfers due to sync issues.

[c4-submissions]

Lines of code

https://github.com/code-423n4/2023-11-zetachain/blob/2834e3f85b2c7774e97413936018a0814c57d860/repos/protocol-contracts/contracts/zevm/WZETA.sol#L47 https://github.com/code-423n4/2023-11-zetachain/blob/2834e3f85b2c7774e97413936018a0814c57d860/repos/protocol-contracts/contracts/zevm/WZETA.sol#L46-L60

Vulnerability details

Impact

There is a check that happens on this line of transferFrom(): WZETA.sol#Line 47

require(balanceOf[src] >= wad);

This checks that the source address has enough token balance before allowing transfer. If balanceOf mappings get out of sync with actual token balances, this check could allow over-transfers.

Here are some potential causes for synchronization issues:

• Integer overflow/underflow bugs leading to incorrect balanceOf increments/decrements
• Mapping variable manipulation vulnerability allowing directly editing balances
• Issues with token cloning or contract upgrades leading to balance replication problems

Imagine this scenario

1. Contract has 100 actual tokens, Alice has a balanceOf 100 tokens
2. Due to an integer overflow, a previous deposit wrongly increments Alice's balanceOf to 300
3. Alice tries to transfer 200 tokens to Bob
4. The balanceOf check passes, as her balanceOf still shows 300
5. But there are only actually 100 tokens in the contract
6. So Alice successfully transfers more tokens than are available

Effects

• Token balances no longer match actual token holdings
• Appearance of "phantom" tokens
• Other users balances appear reduced

Proof of Concept

The key transfer logic happens in transferFrom() of WZETA.sol

    function transferFrom(address src, address dst, uint wad) public returns (bool) {

        // Balance check
        require(balanceOf[src] >= wad);  <---- The specific code related to the vulnerability is
        // This requires the source address has sufficient token balance to cover the transfer.

        if (src != msg.sender && allowance[src][msg.sender] != uint(-1)) {
            require(allowance[src][msg.sender] >= wad);
            allowance[src][msg.sender] -= wad;
        }

        // Transfer
        balanceOf[src] -= wad;
        balanceOf[dst] += wad;

        // Event
        Transfer(src, dst, wad);

        return true;
    }

If balanceOf becomes inconsistent with actual balances, this check could allow larger transfers than should be possible, enabling theft. out-of-sync balanceOf could trick this check into allowing more tokens transferred out than the contract actually holds.

Imagine this concept showing how an out-of-sync mapping could lead to over-transfers.

mapping (address => uint) public balanceOf; 
uint actualTotalSupply = 100;

function faultyIncrementBalance (address user, uint amount) external {
  // Bad logic doesn't check balances
  balanceOf[user] += amount;  
}

function transfer(address to, uint amount) external {
  require(balanceOf[msg.sender] >= amount);
  balanceOf[msg.sender] -= amount;
  balanceOf[to] += amount; 
}

1. Alice has 100 tokens
2. Actual total supply is 100.
3. Faulty func wrongly increments Alice's balanceOf to 300
4. Alice's balanceOf now 300 even though real balance 100
5. Alice tries to transfer 200 to Bob
6. Check balanceOf[msg.sender] >= amount passes since 300 >= 200
7. Allowed to transfer 200 even with only 100 actual tokens!

If the mappings get out of sync, the transfer check could be tricked into allowing more assets out than the contract holds.

Tools Used

Vs Code

Recommended Mitigation Steps

This are my recommended ways to mitigate the risk of balanceOf mappings getting out-of-sync.

First is to use a single source of truth Rather than relying solely on the balanceOf mappings, maintain one canonical source of token balances, like a totalSupply variable that tracks the actual number of tokens in the contract.

All balance-changing operations would check against this versus simply incrementing/decrementing mappings.

Then secondly, Access control balanceOf modifications Protect the ability to modify sensitive balanceOf state. Make sure only authorized addresses (like owner) can call functions that update balances.

And the last resort if to use SafeMath When making balance increments/decrements, use SafeMath operations which automatically prevent/catch over/underflows that could desync values.

For example:

using SafeMath for uint;

balanceOf[user] = balanceOf[user].sub(amount); 

Assessed type

Token-Transfer

[c4-pre-sort]

DadeKuma marked the issue as insufficient quality report

[c4-judge]

0xean marked the issue as unsatisfactory: Insufficient quality