Malicious or hacked admin can steal all ETH

[code423n4]

Lines of code

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

Vulnerability details

Impact

In L2EthToken.sol we have transferFromTo() It is possible malicious or hacked admin to steal the ETH.

Proof of Concept

As can be seen from the code snippet below, nothing can stop malicious or hacked admin to steal all ETH. He can use address _from and send the ETH to address _to. I see from the NatSpec that function can be called only by trusted system contracts, but for greater safety it is good to add timelock mechanism.

function transferFromTo(address _from, address _to, uint256 _amount) external override {
        require(
            msg.sender == MSG_VALUE_SYSTEM_CONTRACT ||
                msg.sender == address(DEPLOYER_SYSTEM_CONTRACT) ||
                msg.sender == BOOTLOADER_FORMAL_ADDRESS,
            "Only system contracts with special access can call this method"
        );

        uint256 fromBalance = balance[_from];
        require(fromBalance >= _amount, "Transfer amount exceeds balance");
        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;
        }

        emit Transfer(_from, _to, _amount);
    }

Tools Used

Manual Review

Recommended Mitigation Steps

For more certain it is good to add timelock mechanism.

---

[c4-judge]

GalloDaSballo marked the issue as primary issue

[c4-sponsor]

vladbochok marked the issue as sponsor disputed

[vladbochok]

As can be seen from the code snippet below, nothing can stop malicious or hacked admin to steal all ETH.

It is not true. The function can be called only by the list of the system contracts, the implementation of which was included in the scope of this contest. How exactly malicious or hacked admin could steal users' funds?

I see from the NatSpec that function can be called only by trusted system contracts, but for greater safety it is good to add timelock mechanism.

That doesn't make sense, since the transferFromTo function is used by the contracts to change ether account balances.

[c4-sponsor]

vladbochok requested judge review

[GalloDaSballo]

Agree with the sponsor, the code-path are in the in-scope codebase and there's no rational code path that allows to move ETH from the admin

[c4-judge]

GalloDaSballo marked the issue as unsatisfactory: Insufficient proof