Loss of funds when msg.value > 2**128

[code423n4]

Lines of code

https://github.com/code-423n4/2023-03-zksync/blob/21d9a364a4a75adfa6f1e038232d8c0f39858a64/contracts/MsgValueSimulator.sol#L48-L57

Vulnerability details

Impact

When a user try to transfer an amount of ether > 2**128 an invariant is broken where instead of reverting the transaction the affected code just return.

Proof of Concept

The following is the affected code where it returns instead of reverting.

        if (value != 0) {
            (bool success, ) = address(ETH_TOKEN_SYSTEM_CONTRACT).call(
                abi.encodeCall(ETH_TOKEN_SYSTEM_CONTRACT.transferFromTo, (msg.sender, to, value))
            );
        ...
        if (value > MAX_MSG_VALUE) {
            // The if above should never be true, since noone should be able to have
            // MAX_MSG_VALUE wei of ether. However, if it does happen for some reason,
            // we will revert(0,0).
            // Note, that we use raw revert here instead of `panic` to emulate behaviour close to
            // the EVM's one, i.e. returndata should be empty.
            assembly {
                return(0, 0)
            }
        }

Note that the amount of ether would be locked in the MsgValueSimulator contract because by returning the state changes are not reverted.

The above issue affects everywhere in the codebase where a transfer of ether happens. For example when calling a constructor and passing ether there is a cast which would be safe due to the above check if correct. As a consequence an amount of ether equal to 2**128 could be lost.

    function _constructContract(address _sender, address _newAddress, bytes calldata _input, bool _isSystem) internal {
        // Transfer the balance to the new address on the constructor call.
        uint256 value = msg.value;
        if (value > 0) {
            ETH_TOKEN_SYSTEM_CONTRACT.transferFromTo(address(this), _newAddress, value);
            // Safe to cast value, because `msg.value` <= `uint128.max` due to `MessageValueSimulator` invariant
            SystemContractHelper.setValueForNextFarCall(uint128(value));
        }

Other places affected are the rawCall and the systemCall functions.

Recommended Mitigation Steps

Revert instad of returning.

[GalloDaSballo]

Not enough ETH

[c4-judge]

GalloDaSballo marked the issue as unsatisfactory: Invalid