zkSync Era - Dup1337's results

Lines of code

https://github.com/code-423n4/2024-03-zksync/blob/4f0ba34f34a864c354c7e8c47643ed8f4a250e13/code/contracts/ethereum/contracts/state-transition/StateTransitionManager.sol#L165-L167

Vulnerability details

The StateTransitionManager contract in the zkSync system contains two functions, freezeChain and unfreezeChain, intended to manage the operational state of a specified blockchain by freezing and unfreezing it, respectively. Both functions are designed to either halt or resume operations, particularly in response to detected vulnerabilities or the need for maintenance. However, unfreezeChain() function incorrectly calls freezeDiamond() instead of an expected counterpart method unfreezeDiamond() that would revert the chain's state. This means that chain that is once frozen, is blocked forever.

Impact

Permament DoS of a chain after first freeze.

Proof of Concept

Please add following test to FreezeChain.t.sol and execute it:

    function test_FreezingUnfreezingChain() public {
        createNewChain(getDiamondCutData(diamondInit));

        address newChainAddress = chainContractAddress.stateTransition(chainId);
        GettersFacet gettersFacet = GettersFacet(newChainAddress);
        bool isChainFrozen = gettersFacet.isDiamondStorageFrozen();
        assertEq(isChainFrozen, false);

        vm.stopPrank();
        vm.startPrank(governor);

        chainContractAddress.freezeChain(block.chainid);
        vm.expectRevert(bytes.concat("q1")); // storage frozen, cannot unfreeze
        chainContractAddress.unfreezeChain(block.chainid);
    }

The tests runs successfully, meaning that it reverts with q1 error - storage frozen:

Running 1 test for test/foundry/unit/concrete/state-transition/StateTransitionManager/FreezeChain.t.sol:freezeChainTest
[PASS] test_FreezingUnfreezingChain() (gas: 3397331)
Test result: ok. 1 passed; 0 failed; 0 skipped; finished in 6.46ms

Ran 1 test suites: 1 tests passed, 0 failed, 0 skipped (1 total tests)

Tools Used

Manual Review

Recommended Mitigation Steps

Change code to unfreeze diamond:

function unfreezeChain(uint256 _chainId) external onlyOwner {
-    IZkSyncStateTransition(stateTransition[_chainId]).freezeDiamond();
+    IZkSyncStateTransition(stateTransition[_chainId]).unfreezeDiamond();
}

Assessed type

DoS

[L-01] Funds lost when hyperbridging is enabled after adding chains

hyperbridgingEnabled is defined and used in code, however, it's not set. The sponsor confirmed that it's a subject of introduction in the next update. There is one possible issue that might arise, when the hyperbridging is enabled sometime after a chain is running, concerning chainBalance increasing on deposits and decreasing on withdrawals/failed deposit claims. By default hyperbridgingEnabled is set to false and chainBalance won't be increased on deposits, when it's set to false later, it might DoS withdrawal:

    function _claimFailedDeposit(
        bool _checkedInLegacyBridge,
        uint256 _chainId,
        address _depositSender,
        address _l1Token,
        uint256 _amount,
        bytes32 _l2TxHash,
        uint256 _l2BatchNumber,
        uint256 _l2MessageIndex,
        uint16 _l2TxNumberInBatch,
        bytes32[] calldata _merkleProof
    ) internal nonReentrant {
        // [...]

        if (!hyperbridgingEnabled[_chainId]) { // @audit possible DoS when it will be set after user deposits are made
            // check that the chain has sufficient balance
            require(chainBalance[_chainId][_l1Token] >= _amount, "ShB n funds");
            chainBalance[_chainId][_l1Token] -= _amount;
        }

[L-02] Unable to claim failed deposit when requestL2TransactionDirect() is used directly

There are 2 possibilities to deposit funds into Hyperbrige: via L1SharedBridge, or legacy L1ERC20Bridge. In both cases L1SharedBridge.depositHappened struct is updated to allow the user to claim failed deposit. the legacy bridge uses requestL2TransactionDirect() and doesn't call confirmL2Transaction - it's meant to be called from the legacy bridge, which keeps deposit data, that's also why there's this check in L1SharedBridge :

notCheckedInLegacyBridgeOrWeCanCheckDeposit = (!_checkedInLegacyBridge) || weCanCheckDepositHere;

However, L1SharedBridge.requestL2TransactionDirect() can also be called by anyone directly, without registering information about deposit, hence not allowing to claim failed deposit in case of issues.

Please consider either limiting requestL2TransactionDirect() to L1ERC20Bridge caller or at least mark info about possible funds loss when using this function in docs.

[L-03] No possibility to know if an immutable exists or not

In zkSync, there is no notion of constructor bootstrapping contract, there is no possibility to override smart contract code during deployment, which is used in immutable variables.

Because zkSync still wants to support them, the implementation comes up with a smart contract that simulates immutables. However, when calling getImmutable, it returns a mapping value of an index. When the value there is not initialized, it will return 0, just as the value initialized with 0, so there is no way of knowing if a certain immutable was set for a given contract.

Contract: ImmutableSimulator.sol

18: contract ImmutableSimulator is IImmutableSimulator {
19:     /// @dev mapping (contract address) => (index of immutable variable) => value
20:     /// @notice that address uses `uint256` type to leave the option to introduce 32-byte address space in future.
21:     mapping(uint256 contractAddress => mapping(uint256 index => bytes32 value)) internal immutableDataStorage;
22: 
23:     /// @notice Method that returns the immutable with a certain index for a user.
24:     /// @param _dest The address which the immutable belongs to.
25:     /// @param _index The index of the immutable.
26:     /// @return The value of the immutables.

///  @audit Because there is no check if the index is set, it will return 0 on both occasions that the immutable exists and is 0, and the immutable index not existing
27:     function getImmutable(address _dest, uint256 _index) external view override returns (bytes32) {
28:         return immutableDataStorage[uint256(uint160(_dest))][_index];
29:     }
30: 
31:     /// @notice Method used by the contract deployer to store the immutables for an account
32:     /// @param _dest The address which to store the immutables for.
33:     /// @param _immutables The list of the immutables.
34:     function setImmutables(address _dest, ImmutableData[] calldata _immutables) external override {
35:         require(msg.sender == address(DEPLOYER_SYSTEM_CONTRACT), "Callable only by the deployer system contract");
36:         unchecked {
37:             uint256 immutablesLength = _immutables.length;
38:             for (uint256 i = 0; i < immutablesLength; ++i) {
39:                 uint256 index = _immutables[i].index;
40:                 bytes32 value = _immutables[i].value;
41:                 immutableDataStorage[uint256(uint160(_dest))][index] = value;
42:             }
43:         }
44:     }
45: }
46: 

[L-04] NonceHolder can´t set a value for the first nonce

setValueUnderNonce sets the nonce value key for the msg.sender as used;

Contract: NonceHolder.sol

82:     function setValueUnderNonce(uint256 _key, uint256 _value) public onlySystemCall {
83:         IContractDeployer.AccountInfo memory accountInfo = DEPLOYER_SYSTEM_CONTRACT.getAccountInfo(msg.sender);
84: 
85:         require(_value != 0, "Nonce value cannot be set to 0");
86:         // If an account has sequential nonce ordering, we enforce that the previous
87:         // nonce has already been used.
88:         if (accountInfo.nonceOrdering == IContractDeployer.AccountNonceOrdering.Sequential && _key != 0) {
89:             require(isNonceUsed(msg.sender, _key - 1), "Previous nonce has not been used"); 
90:         }
91: 
92:         uint256 addressAsKey = uint256(uint160(msg.sender));
93: 
94:         nonceValues[addressAsKey][_key] = _value;
95: 
96:         emit ValueSetUnderNonce(msg.sender, _key, _value);
97:     }
98: 

The call checks the previous key (nonce) is being used if the account nonce mode set in sequential by isNonceUsed(msg.sender, _key - 1)

and isNonceUsed function is below:

Contract: NonceHolder.sol

154:     function isNonceUsed(address _address, uint256 _nonce) public view returns (bool) {
155:         uint256 addressAsKey = uint256(uint160(_address));
156:         return (_nonce < getMinNonce(_address) || nonceValues[addressAsKey][_nonce] > 0);
157:     }

Accordingly, either the previous nonce should be written with a value : nonceValues[addressAsKey][_nonce] > 0 OR getMinNonce(_address) should be greater than the nonce:

Contract: NonceHolder.sol

46:     function getMinNonce(address _address) public view returns (uint256) {
47:         uint256 addressAsKey = uint256(uint160(_address));
48:         (, uint256 minNonce) = _splitRawNonce(rawNonces[addressAsKey]);
49: 
50:         return minNonce;
51:     }

Whereas _splitRawNonce is:

Contract: NonceHolder.sol

179:     function _splitRawNonce(uint256 _rawMinNonce) internal pure returns (uint256 deploymentNonce, uint256 minNonce) {
180:         deploymentNonce = _rawMinNonce / DEPLOY_NONCE_MULTIPLIER;
181:         minNonce = _rawMinNonce % DEPLOY_NONCE_MULTIPLIER;
182:     }

So, if line 181 returns 0 - the first nonce after deployment, it wont suffice the requirement of _nonce < getMinNonce(_address) (L:156 of NonceHolder.sol)

Moreover, in this line:

Contract: NonceHolder.sol

88:         if (accountInfo.nonceOrdering == IContractDeployer.AccountNonceOrdering.Sequential && _key != 0) {
89:             require(isNonceUsed(msg.sender, _key - 1), "Previous nonce has not been used"); 
90:         }

if the previous nonce is used - as desired - it continues to write the proposed nonce,

BUT, It´s never checked whether the proposed nonce is USED either, which is recommended to be validated before writing it.

[L-05] onlySelf modifier causes bypassing access control

Governance contract´s onlySelf modifier is used in updateDelay and updateSecurityCouncil functions. Accordingly, a scheduled upgrade can be executed either in execute or executeInstant functions to change the minDelay and securityCouncil variables. These execution functions will set the caller context as msg.sender == address(this).

Here´s the onlySelf modifier:

Contract: Governance.sol

58:     modifier onlySelf() {
59:         require(msg.sender == address(this), "Only governance contract itself is allowed to call this function");
60:         _;
61:     }

However, by this, the Owner can change the SecurityCouncil arbitrarily due to: a. The owner can call scheduleTransparent and scheduleShadow b. The owner can execute the scheduled operation by execute

So, the functions with onlySecurityCouncil can be bypassed especially when the Owner calls scheduleShadow with the calldata of updating the Security Council to their address and execute the operation on their own.

After the change of Security Council, any operation can be proposed and executed at the same TX instantly due to there is no vetoer body.

We recommend implementing a flag for scheduleShadow. Accordingly, don´t let the owner execute if the Operation being executed is a shadow schedule and the caller is the owner:

Contract: Governance.sol

167:     function execute(Operation calldata _operation) external payable onlyOwnerOrSecurityCouncil {
168:         bytes32 id = hashOperation(_operation);
169:         // Check if the predecessor operation is completed.
170:         _checkPredecessorDone(_operation.predecessor);
171:         // Ensure that the operation is ready to proceed.
172:         require(isOperationReady(id), "Operation must be ready before execution");//@audit-ok this can be executed earlier!
+             if (msg.sender == owner) {
+             require(!isShadowSchedule(id));
+             }
173:         _execute(_operation.calls);
174:         // Reconfirming that the operation is still ready after execution.
175:         // This is needed to avoid unexpected reentrancy attacks of re-executing the same operation.
176:         require(isOperationReady(id), "Operation must be ready after execution"); // @audit-ok this will pass anyway, not sure about the logic
177:         // Set operation to be done
178:         timestamps[id] = EXECUTED_PROPOSAL_TIMESTAMP; 
179:         emit OperationExecuted(id);
180:     }

[L-06] Governance execute and executeInstant functions to follow CEI pattern

Both execute and executeInstant functions a. execute the calls b. checks for re-entrancy c. Marks the id with EXECUTED_PROPOSAL_TIMESTAMP

However, the below changes are more convenient:

Contract: Governance.sol

167:     function execute(Operation calldata _operation) external payable onlyOwnerOrSecurityCouncil {
168:         bytes32 id = hashOperation(_operation);
169:         // Check if the predecessor operation is completed.
170:         _checkPredecessorDone(_operation.predecessor);
171:         // Ensure that the operation is ready to proceed.
172:         require(isOperationReady(id), "Operation must be ready before execution");
+            // Set operation to be done
+            timestamps[id] = EXECUTED_PROPOSAL_TIMESTAMP; 
173:         _execute(_operation.calls);
-            // Reconfirming that the operation is still ready after execution.
-            // This is needed to avoid unexpected reentrancy attacks of re-executing the same operation.
-            require(isOperationReady(id), "Operation must be ready after execution"); 
-            // Set operation to be done
-            timestamps[id] = EXECUTED_PROPOSAL_TIMESTAMP; 
179:         emit OperationExecuted(id);
180:     }

Contract: Governance.sol

185:     function executeInstant(Operation calldata _operation) external payable onlySecurityCouncil { 
186:         bytes32 id = hashOperation(_operation);
187:         // Check if the predecessor operation is completed.
188:         _checkPredecessorDone(_operation.predecessor);
189:         // Ensure that the operation is in a pending state before proceeding.
190:         require(isOperationPending(id), "Operation must be pending before execution"); 
+            // Set operation to be done
+            timestamps[id] = EXECUTED_PROPOSAL_TIMESTAMP;
191:         // Execute operation.
192:         _execute(_operation.calls);
-            // Reconfirming that the operation is still pending before execution.
-            // This is needed to avoid unexpected reentrancy attacks of re-executing the same operation.
-            require(isOperationPending(id), "Operation must be pending after execution");
-            // Set operation to be done
-            timestamps[id] = EXECUTED_PROPOSAL_TIMESTAMP;
198:         emit OperationExecuted(id);
199:     } 

[L-07] The operator will not be paid any fees when refundGas == gasLimit

During processing L1 TX in Bootloader the surplus fees can be refunded to the user depending on the gasLimit and the gas price. This is calculated by the operator and the Bootloader and accordingly, the user is refunded.

The snippet shows the brief logic of the mechanism:

Contract: bootloader.yul

 985:                     let potentialRefund := saturatingSub(
 986:                         safeAdd(reservedGas, gasForExecution, "safeadd: potentialRefund1"), 
 987:                         safeAdd(gasSpentOnExecution, ergsSpentOnPubdata, "safeadd: potentialRefund2")
 988:                     )
 989: 
 990:                     // Asking the operator for refund
 991:                     askOperatorForRefund(potentialRefund, ergsSpentOnPubdata, gasPerPubdata)
 992: 
 993:                     // In case the operator provided smaller refund than the one calculated
 994:                     // by the bootloader, we return the refund calculated by the bootloader.
 995:                     refundGas := max(getOperatorRefundForTx(transactionIndex), potentialRefund) 
 996:                 }
 997: 
 998:                 if gt(refundGas, gasLimit) {
 999:                     assertionError("L1: refundGas > gasLimit")
1000:                 }
1001: 
1002:                 let payToOperator := safeMul(gasPrice, safeSub(gasLimit, refundGas, "lpah"), "mnk") 
1003: 
1004:                 notifyAboutRefund(refundGas)
1005: 
1006:                 // Paying the fee to the operator
1007:                 mintEther(BOOTLOADER_FORMAL_ADDR(), payToOperator, false)
1008: 

L: 991 - 995 calculates the refund if any.

L: 998 ensures that the refundGas should not be more than gasLimit to prevent an infinite money glitch.

L: 1002 calculates the fee amount to be paid (minted) to the node. However, the operator will not receive anything when gasLimit == refundGas due to gasPrice * (gasLimit - refundGas) == 0

We recommend asserting payToOperator > 0 condition to eliminate fee bypassing.

[L-08] Tests fail silently

During testing we found a test "Should deposit erc20 token successfully", that succeeds, however, it silently fails. It can be verified by adding a revert reason for the test case:

  it("Should deposit erc20 token successfully", async () => {
    const amount = ethers.utils.parseEther("0.001");
    const mintValue = ethers.utils.parseEther("0.002");
    await l1Weth.connect(randomSigner).deposit({ value: amount });
    await (await l1Weth.connect(randomSigner).approve(l1SharedBridge.address, amount)).wait();
+   const revertReason = await getCallRevertReason(
    bridgehub.connect(randomSigner).requestL2TransactionTwoBridges(
      {
        chainId,
        mintValue,
        l2Value: amount,
        l2GasLimit: 1000000,
        l2GasPerPubdataByteLimit: REQUIRED_L2_GAS_PRICE_PER_PUBDATA,
        refundRecipient: ethers.constants.AddressZero,
        secondBridgeAddress: l1SharedBridge.address,
        secondBridgeValue: 0,
        secondBridgeCalldata: new ethers.utils.AbiCoder().encode(
          ["address", "uint256", "address"],
          [l1Weth.address, amount, await randomSigner.getAddress()]
        ),
      },
      { value: mintValue }
+   )
    );
+   expect(revertReason).equal("ShB: WETH deposit not supported");
  });

It succeds, even though it shouldn't

  Shared Bridge tests
   ✔ Should deposit erc20 token successfully
  103 passing (53s)

It succeeds. That means that the test really doesn't do what it's supposed to. Because there are many places with similar testing patterns, we encourage the team to go through the test cases and make sure that the tests really test what they are supposed to.

[NC-01] Withdrawals don´t have a cool down period

The zKsync unique architecture provides almost instant L1 <-> L2 interactions compared to other roll ups. While it´s a big advantage, it has a trade-off not being able to intervene worst-case scenarios.

The finalizeWithdrawal function serves almost instantly once the required data is provided. However, this will be an escape hatch for the black hats as withdrawals of the theft funds from L2 to L1 will be smooth compared to other roll ups, despite having a privileged function called revertBatches.

We recommend considering implementing a cool down period as applicable as CEX implementations, so that the fauled batch can be reverted by the privileged role (Admin / Owner)

[NC-02] Shadow Upgrade calldata will be on-chain during executeInstant and subject to frontruns

The scheduleShadow function schedules an upgrade only by the id and prevents the calldata from being studied.

Contract: Governance.sol

142:     function scheduleShadow(bytes32 _id, uint256 _delay) external onlyOwner {
143:         _schedule(_id, _delay);
144:         emit ShadowOperationScheduled(_id, _delay);
145:     }
146: 

And the SecurityCouncil executes the upgrade by executeInstant function:

Contract: Governance.sol

185:     function executeInstant(Operation calldata _operation) external payable onlySecurityCouncil { 
186:         bytes32 id = hashOperation(_operation);
187:         // Check if the predecessor operation is completed.
188:         _checkPredecessorDone(_operation.predecessor);
189:         // Ensure that the operation is in a pending state before proceeding.
190:         require(isOperationPending(id), "Operation must be pending before execution"); 
191:         // Execute operation.
192:         _execute(_operation.calls);
193:         // Reconfirming that the operation is still pending before execution.
194:         // This is needed to avoid unexpected reentrancy attacks of re-executing the same operation.
195:         require(isOperationPending(id), "Operation must be pending after execution");
196:         // Set operation to be done
197:         timestamps[id] = EXECUTED_PROPOSAL_TIMESTAMP;
198:         emit OperationExecuted(id);
199:     }

But since the Operation calldata is the input, it has a MEV risk anyway.

[NC-03] MsgValueSimulator may be forced to revert

MsgValueSimulator checks how much gas was passed, and it reserves MSG_VALUE_SIMULATOR_STIPEND_GAS for the call:

    fallback(bytes calldata _data) external onlySystemCall returns (bytes memory) {
        // Firstly we calculate how much gas has been actually provided by the user to the inner call.
        // For that, we need to get the total gas available in this context and subtract the stipend from it.
        uint256 gasInContext = gasleft();
        // Note, that the `gasInContext` might be slightly less than the MSG_VALUE_SIMULATOR_STIPEND_GAS, since
        // by the time we retrieve it, some gas might have already been spent, e.g. on the `gasleft` opcode itself.
        // @audit gasInContext might be set to a specific value that will 
        uint256 userGas = gasInContext > MSG_VALUE_SIMULATOR_STIPEND_GAS
            ? gasInContext - MSG_VALUE_SIMULATOR_STIPEND_GAS
            : 0;

Technically it's possible to set gasleft specifically to 27001 (MSG_VALUE_SIMULATOR_STIPEND_GAS = 27000), so that given that the gas is substracted during userGas calculations, actual gas will be smaller than 27000, because of the costs of opcode execution after gasleft is cached in gasInContext. While currently, we haven't identified any possibilities of doing so, please consider adding additional gas.