code-423n4 / 2023-10-zksync-findings

3 stars 0 forks source link

Operators May Drain All User Gas Without Providing Tx Execution #309

Closed c4-submissions closed 11 months ago

c4-submissions commented 1 year ago

Lines of code

https://github.com/code-423n4/2023-10-zksync/blob/1fb4649b612fac7b4ee613df6f6b7d921ddd6b0d/code/system-contracts/bootloader/bootloader.yul#L1241-L1277 https://github.com/code-423n4/2023-10-zksync/blob/1fb4649b612fac7b4ee613df6f6b7d921ddd6b0d/code/system-contracts/contracts/Compressor.sol#L54-L82 https://github.com/code-423n4/2023-10-zksync/blob/1fb4649b612fac7b4ee613df6f6b7d921ddd6b0d/code/system-contracts/contracts/L1Messenger.sol#L153-L159

Vulnerability details

Impact

Operators are able to cause EIP-712 transactions with factory dependencies to consume all of the gas without executing the transaction. Thus, the user will pay the full price of the transaction (gasPrice * gasLimit) without receiving any execution.

Note that this attack will not update any state in L1Mesenger, Compressor or KnownStorageCodes since the near call is reverted.

The issue is rated as medium severity, while it does allow stealing user gasPrice * gasLimit worth of tokens without providing a service it requires the following assumptions. a) Attacker must be the operator (who also receives the gas payment) b) User has signed an EIP-712 transaction which has at least 1 factory dependency that is unmarked.

Proof of Concept

Within the function l2TxExecution() if the near call ZKSYNC_NEAR_CALL_markFactoryDepsL2() consumes more than gasLeft then the transaction will not be executed.

            function l2TxExecution(
                txDataOffset,
                gasLeft,
            ) -> success, gasSpentOnExecute {
                let newCompressedFactoryDepsPointer := 0
                let gasSpentOnFactoryDeps := 0
                let gasBeforeFactoryDeps := gas()
                if gasLeft {
                    let markingDependenciesABI := getNearCallABI(gasLeft)
                    checkEnoughGas(gasLeft)
                    newCompressedFactoryDepsPointer := ZKSYNC_NEAR_CALL_markFactoryDepsL2(markingDependenciesABI, txDataOffset) //@audit can be manipulated to consumer more than `gasLeft`
                    gasSpentOnFactoryDeps := sub(gasBeforeFactoryDeps, gas())
                }

                // If marking of factory dependencies has been unsuccessful, 0 value is returned.
                // Otherwise, all the previous dependencies have been successfully published, so
                // we need to move the pointer.
                if newCompressedFactoryDepsPointer {
                    mstore(COMPRESSED_BYTECODES_BEGIN_BYTE(), newCompressedFactoryDepsPointer)
                }

                switch gt(gasLeft, gasSpentOnFactoryDeps) 
                case 0 { // @audit this case is trigger and execution does not occur
                    gasSpentOnExecute := gasLeft
                    gasLeft := 0
                }
                default {
                    // Note, that since gt(gasLeft, gasSpentOnFactoryDeps) = true
                    // sub(gasLeft, gasSpentOnFactoryDeps) > 0, which is important
                    // because a nearCall with 0 ergs passes on all the ergs of the parent frame.
                    gasLeft := sub(gasLeft, gasSpentOnFactoryDeps)

                    let executeABI := getNearCallABI(gasLeft)
                    checkEnoughGas(gasLeft)

                    let gasBeforeExecute := gas()
                    // for this one, we don't care whether or not it fails.
                    success := ZKSYNC_NEAR_CALL_executeL2Tx( //@audit this transaction body is not executed
                        executeABI,
                        txDataOffset
                    )

                    gasSpentOnExecute := add(gasSpentOnFactoryDeps, sub(gasBeforeExecute, gas()))
                }

                notifyExecutionResult(success)
            }

It is possible for an operator to exploit the call to ZKSYNC_NEAR_CALL_markFactoryDepsL2() to consume more than gasLeft amount of gas and cause a near call revert. In the function ZKSYNC_NEAR_CALL_markFactoryDepsL2() there is a sub-call to sendCompressedBytecode() if the bytecode is not yet marked as known.

The function sendCompressedBytecode() will call Compressor.publishCompressedBytecode() with calldata read from just after COMPRESSED_BYTECODES_BEGIN_BYTE() which is provided by the operator.

The operator is able to arbitrarily set the calldata except for the function selector which is set by the bootloader. The attack is for the operator to set _rawCompressedData such that it decodes a dictionary with very large length (e.g. 2^16 - 1). Since publishCompressedBytecode() does not put any restrictions on the length of dictionary other than it is less than (2^16 - 1) * 8.

    function publishCompressedBytecode(
        bytes calldata _bytecode, 
        bytes calldata _rawCompressedData //@audit arbitrary input from operator
    ) external payable onlyCallFromBootloader returns (bytes32 bytecodeHash) {
        unchecked {
            (bytes calldata dictionary, bytes calldata encodedData) = _decodeRawBytecode(_rawCompressedData);

            require(dictionary.length % 8 == 0, "Dictionary length should be a multiple of 8");
            require(dictionary.length <= 2 ** 16 * 8, "Dictionary is too big");
            require(
                encodedData.length * 4 == _bytecode.length,
                "Encoded data length should be 4 times shorter than the original bytecode"
            );

            for (uint256 encodedDataPointer = 0; encodedDataPointer < encodedData.length; encodedDataPointer += 2) {
                uint256 indexOfEncodedChunk = uint256(encodedData.readUint16(encodedDataPointer)) * 8;
                require(indexOfEncodedChunk < dictionary.length, "Encoded chunk index is out of bounds");

                uint64 encodedChunk = dictionary.readUint64(indexOfEncodedChunk);
                uint64 realChunk = _bytecode.readUint64(encodedDataPointer * 4);

                require(encodedChunk == realChunk, "Encoded chunk does not match the original bytecode");
            }
        }

        bytecodeHash = Utils.hashL2Bytecode(_bytecode);
        L1_MESSENGER_CONTRACT.sendToL1(_rawCompressedData);
        KNOWN_CODE_STORAGE_CONTRACT.markBytecodeAsPublished(bytecodeHash);
    }
function _decodeRawBytecode(
        bytes calldata _rawCompressedData
    ) internal pure returns (bytes calldata dictionary, bytes calldata encodedData) {
        unchecked {
            // The dictionary length can't be more than 2^16, so it fits into 2 bytes.
            uint256 dictionaryLen = uint256(_rawCompressedData.readUint16(0)); //@audit upper bounds is 2^16 - 1
            dictionary = _rawCompressedData[2:2 + dictionaryLen * 8];
            encodedData = _rawCompressedData[2 + dictionaryLen * 8:];
        }
    }

For example consider the following scenario where the user bytecode is _bytecode = 0x1111111111111111111111111111111111111111111111111111111111111111 (i.e. 32 bytes of 0x11). The attacker can create a valid "compression" which is 74 bytes long. In this example we have dictionaryLen = 0x0008 which is 8 * 8 bytes, the dictionary has the eight bytes 0x1111111111111111 and the rest zeroes. We then have encodedData = 0x0000000000000000 where each index points to the first element of the dictionary 0x11111111111111111. _rawCompressedData = 0x0008111111111111111100000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000.

This is in contrast to the optimal encoding which would be _rawCompressedData = 0x000111111111111111110000000000000000 and is 18 bytes in length.

The function function publishCompressedBytecode() then calls to the system contract L1_MESSENGER_CONTRACT.sendToL1(_rawCompressedData); where the user is charged gas proportional to the length of _rawCompressedData as seen in the following snippet where pubdataLen = _rawCompressedData.length.

        uint256 gasToPay = pubdataLen *
            gasPerPubdataBytes +
            keccakGasCost(L2_TO_L1_LOG_SERIALIZE_SIZE) +
            4 *
            keccakGasCost(64) +
            gasSpentOnMessageHashing;
        SystemContractHelper.burnGas(Utils.safeCastToU32(gasToPay)); //@audit gas is burnt based off the size of pubdata

The operator exploits this bug by providing the system with a _rawCompressedData which is valid but significantly larger than necessary. This will charge the user gas proportionate to the excess data. If the operator designs the parameters such that the gas burnt is larger than the remaining gas this call will revert thereby undoing any state changes or events (the L2 to L1 message will not be processed).

To summarize, a malicious operator sets a very large dictionary in the factory dependencies input. This consumes all user transaction gas within the near call ZKSYNC_NEAR_CALL_markFactoryDepsL2(). As a result the user transaction is not executed and contract bytecode is not sent to L1, however the user is still charged gasPrice * gasLimit.

Tools Used

Manual review.

Recommended Mitigation Steps

The best solution to avoid this attack is to ensure the raw byte code is compressed optimally. To ensure optimality this requires two conditions: a) Each element of the dictionary is read from b) There are no duplicate elements in the dictionary

To achieve both of these conditions, use a uint64 memory array of length equal to dictionary divided by 8 e.g. dictionaryUsed. For each item of dictionary that is read i.e. indexOfEncodedChunk set dictionaryUsed[indexOfEncodedChunk] = encodedChunk.

After looping over encodedData then perform a second loop over dictionaryUsed. Ensure each element of dictionaryUsed is strictly greater than the previous. This requires a sorted dictionary and ensures both conditions. First, each element of the dictionary is read from otherwise dictionaryUsed[i] = 0 and will be less than the previous element. Additionally, it prevents duplicates since each element must be strictly greater than the previous.

This is not a gas efficient solution. An efficient solution which is less effective could be to ensure _rawCompressedData < _bytecode. This will prevent some manipulation by the operator but limit the maximum impact.

Assessed type

Invalid Validation

c4-pre-sort commented 11 months ago

bytes032 marked the issue as low quality report

miladpiri commented 11 months ago

Duplicate.

c4-judge commented 11 months ago

GalloDaSballo marked the issue as duplicate of #71

c4-judge commented 11 months ago

GalloDaSballo marked the issue as satisfactory