code423n4 commented 1 year ago

Lines of code

https://github.com/code-423n4/2023-03-zksync//blob/main/contracts/L2EthToken.sol#L84-L87

Vulnerability details

Impact

The ethereum balance of the L2EthToken.sol contract can underflow thus breaking the accounting of the protocol for L2-L1 fund transfers.

Proof of Concept

L2EthToken.sol contract has the withdraw function to transfer funds to L1 for withdrawal.

In the withdraw function the internal accounting is done for the amount withdrawn from the contract. The accounting is done inside the unchecked block which means overflow and underflow checks will not be performed for the arithmetic operations happening inside the unchecked block.

The mentioned internal accounting is done as follows in the code:

    unchecked {
        balance[address(this)] -= amount;
        totalSupply -= amount;
    }

In this case the following scenario can happen:

1 . The contract has relatively less funds stored in it due to previous withdrawals. 2 . A particular user who is withdrawing larger value (msg.value) from the contract by calling the withdraw function. 3 . Hence balance[address(this)] < amount can occur. 4 . No check for balance[address(this)] > amount is performed before the internal accounting inside the unchecked block. 5 . The balance[address(this)] -= amount operation can underflow. 6 . If the above condition underflows the balance[address(this)] value will be stored as very large value. 7 . This could further impact the transferFromTo function since following unchecked block can overflow if the address(this) is used as either _from or _to. Since balance[address(this)] will now be a very large value due to previous underflow.

    unchecked {
        balance[_from] = fromBalance - _amount;
        // Overflow not possible: the sum of all balances is capped by totalSupply, and the sum is preserved by
        // decrementing then incrementing.
        balance[_to] += _amount; 
    }

8 . As it is depicted above, this will completely break the entire accounting system of the fund transfers since the contract funds amount is now erroneous.

Tools Used

VS Code and Manual Review

Recommended Mitigation Steps

Perform the balance[address(this)] -= amount calculation outside the unchecked block. So the transaction will revert in the event of an underflow.

c4-judge commented 1 year ago

GalloDaSballo marked the issue as duplicate of #110

c4-sponsor commented 1 year ago

vladbochok marked the issue as sponsor disputed

vladbochok commented 1 year ago

The L2EthToken holds all L2 ether balances. When someone calls the L2EthToken with provided msg.value the balance of the L2EthToken (address(this) in the context of the contract) is increased by that amount. So, it is safe to decrease.

I see this issue as a QA, since there are not enough comments to explain what is going on.