zkSync Era - peanuts's results

Lines of code

https://github.com/code-423n4/2023-10-zksync/blob/1fb4649b612fac7b4ee613df6f6b7d921ddd6b0d/code/contracts/ethereum/contracts/bridge/L1ERC20Bridge.sol#L340-L350

Vulnerability details

Impact

Users will lose their deposits if the transaction from L1->L2 fails and deposit limit is turned on in that duration.

Proof of Concept

When a user calls L1ERC20Bridge.deposit(), _verifyDepositLimit() will be called. _verifyDepositLimit() checks whether the token has a deposit limitation or not. If it does not, return the function. If it does, append the amount deposited to the totalDepositedAmountPerUser mapping.

    function _verifyDepositLimit(address _l1Token, address _depositor, uint256 _amount, bool _claiming) internal {
        IAllowList.Deposit memory limitData = IAllowList(allowList).getTokenDepositLimitData(_l1Token);
        //@audit -- the function will return if there is no deposit limitation
        if (!limitData.depositLimitation) return; // no deposit limitation is placed for this token

        if (_claiming) {
            totalDepositedAmountPerUser[_l1Token][_depositor] -= _amount;
        } else {
            require(totalDepositedAmountPerUser[_l1Token][_depositor] + _amount <= limitData.depositCap, "d1");
            // If there is a limitation, add the total deposited amount
            totalDepositedAmountPerUser[_l1Token][_depositor] += _amount;
        }
    }

    //@audit The deposit struct in IAllowList.sol
    struct Deposit {
        bool depositLimitation;
        uint256 depositCap;
    }

When the transaction from L1-L2 fails, the user can call claimFailedDeposit() to claim back the deposited amount. Likewise, the function calls _verifyDepositLimit() and subtracts the deposited amount from the totalDepositedAmountPerUser struct.

        if (_claiming) {
            totalDepositedAmountPerUser[_l1Token][_depositor] -= _amount;

The issue arises when the deposit limit is turned off at first and then turned on later. For example, User A deposits 10000 DOGE tokens. Since there is no deposit Limit, _verifyDepositLimit() will be skipped.

A second later, the deposit limit for DOGE is turned on and set to 1 million.

User A encounters a failed transaction from L1 to L2. He calls claimFailedDeposit() which calls _verifyDepositLimit(). Since the deposit limit is turned on now, the function runs and the first clause of the if statement is executed because _claiming is true. The user will not get back any tokens because the mapping totalDepositedAmountPerUser is zero and the function will result in underflow.

User A loses 10000 DOGE tokens.

    //@audit Owner can turn the deposit limit on or off using this function in AllowList.sol
    function setDepositLimit(address _l1Token, bool _depositLimitation, uint256 _depositCap) external onlyOwner {
        tokenDeposit[_l1Token].depositLimitation = _depositLimitation;
        tokenDeposit[_l1Token].depositCap = _depositCap;
    }

Tools Used

Manual Review

Recommended Mitigation Steps

Refacor the _verifyDepositLimit() function in such a way that it still stores the amount deposited even if there is no deposit limit

Assessed type

Invalid Validation

1. Summary of the protocol

Zksync era is a new version of the zksync network, which is an L2 that is built on top of Ethereum. In the context of zkSync, an era represents a period where features are implemented or upgraded. The solidity side of the protocol is mostly on the bridge part, where users can bridge their tokens or ETH from L1(Ethereum) to L2(Zksync) and vice versa. During the bridging process, zksync uses zero-knowledge proofs to validate transactions off-chain and submit a SNARK on chain (zero-knowledge Succinct Non-interactive ARgument of Knowledge) to ensure the correctness and security of those transaction. The proofing is done in the circuits part of the protocol using rust.

2. Comments and Disclaimer

Looked through the solidity side of the protocol, mostly bridging, minting, sending transactions and gas calculations. Also looked through the bootloader contract

3. Flow of protocol functionalities

Bridging ERC20 tokens from L1 to L2

• Starts with deposit() in L1ERC20Bridge.sol. User must be in the special allowlist or the access mode is set to public in Allowlist.sol.
• User deposit funds via _depositFunds(). The fund amount must be the same amount as the parameter.
• The user's deposit limit is checked by _verifyDepositLimit()
• _getDepositL2Calldata() is called.
• IL2Bridge.finalizeDeposit is called (in L2ERC20Bridge.sol).
• L2 Token is deployed through _deployL2Token()
• _deployL2Token() calls _deployBeaconProxy and initializes a bridge in L2StandardERC20.sol with bridgeInitialize()
• L2Tokens are minted through bridgeMint() in L2StandardToken.
• Back to L1ERC20Bridge contract, requestL2Transaction() is called in the zkSync Mailbox contract.
• Mailbox.sol, _requestL2Transaction() is called (Take note that _requestL2Transaction can be called directly by the user)
• _requestL2Transaction() calls _writePriorityOp()
• _writePriorityOp() serializes the L2 transaction through _serializeL2Transaction()
• _serializeL2Transaction() calls validateL1ToL2Transaction() and pushes the transaction into the priority queue.
• The function process ends here. The next continuing function calld will be commitBatches() and executeBatch() in Executor.sol

Bridging native tokens from L1 to L2

• Process is similar to bridging ERC20 tokens. Starts with deposit() in L1WethBridge.sol
• The difference is that when _getDepositL2Calldata is called which calls IL2Bridge.finalizeDeposit(), there is no deploying of proxy and minting of tokens
• Instead, the ETH is converted to WETH via IL2Weth(l2WethAddress).depositTo.
• The rest of the process follows a similar pattern as bridging ERC20 tokens

Redeeming a failed deposit on L1 Bridge

• If a transaction fails to be transmitted from L1 to L2, the user can call claimFailedDeposit() to withdraw their locked tokens.
• claimFailedDeposit() calls proveL1ToL2TransactionStatus() in Mailbox.sol contract.
• proveL1ToL2TransactionStatus() gets the L2Log and calls _proveL2LogInclusion().
• _proveL2LogInclusion() calculates the root has and the actual root has and compares them with each other. If both hash are the same, that means the specific L2 log was sent to L2 in a specific L2 batch number.
• Once the transaction is proven, the L1ERC20Bridge claimFailedDeposit() function will verify the deposit limit through _verifyDepositLimit() and transfer the token back to the user

Bridging ERC20 tokens from L2 to L1

• withdraw() is called on L2ERC20Bridge
• bridgeBurn() is called. The L2 ERC20 token is burned first.
• _getL1WithdrawMessage() is called to encode the message
• L2ContractHelper.sendMessageToL1() is called with the encoded message as the parameter.
• L2ContractHelper calls L2_MESSENGER.sendToL1() to send the message to L1
• Seems like the function stops here to prepare for another call.

Bridging native tokens from L2 to L1

• Similar to ERC20 token bridging, withdraw() is called on L2WethBridge
• WETH is burned through IL2StandardToken(l2WethAddress).bridgeBurn
• Native token is sent through L2_ETH_ADDRESS.withdrawWithMessage{value: _amount}(l1Bridge, wethMessage);

4. Codebase Analysis and Review

In-depth Review

Allowlist

• There is quite a bit of centralization risk going on. Most importantly, the owner can set a deposit limit on any ERC20 tokens. If the owner is malicious, he can enable the deposit limit and set it to a really low value to grief the bridging process (no one can bridge any ERC20 tokens).

File: AllowList.sol
129:     function setDepositLimit(address _l1Token, bool _depositLimitation, uint256 _depositCap) external onlyOwner {
130:         tokenDeposit[_l1Token].depositLimitation = _depositLimitation;
131:         tokenDeposit[_l1Token].depositCap = _depositCap;
132:     }

Governance

• The owner can schedule and execute an operation. In every operation, the owner can set a delay before the operation is executable.
• There is also a minDelay variable, which is set in the constructor.
• The security council can execute any operation. The security council can overrule the delay and execute the operation instantly.
• There was an issue in the OpenZeppelin's Timelock Contract whereby an operator can overrule an operation and update the delay and the security council address. This reentrancy issue is mitigated here by checking the state of the operation before and after the execution of the operation
• Reference: https://medium.com/immunefi/openzeppelin-bug-fix-postmortem-66d8c89ed166
• The issue in this contract is that the security council can override a delay but not the owner, which is pretty confusing. If the security council can override, the owner should be able to override too, otherwise there is too much power given to the security council.

L1ERC20Bridge

• Fee-on-transfer token is not supported, which is understandable but may be an issue in the future when more tokens are created with a transfer / burn fee.
• The bridge bridges the exact amount of tokens everytime, so there should not be any leftover amounts in the contract or extra amounts bridged

Mailbox

• Proving transaction / message / logs and also requesting transaction from L1 -> L2
• Apparently requestL2Transaction() can be called directly without going through the L1ERC20 Bridge.
• If called directly, IL2Bridge.finalizeDeposit() is not called, which means that calling requestL2Transaction() directly does not entail bridging tokens.

5. Centralization Risk

6. Learnings

1. In Governance.sol, the operation status must be checked before and after execute, otherwise the check can be bypassed. There was a similar issue with the TimelockController contract. Before the fix, the check was only done after the execute function, which allows the executor to hijack the contract and set the delay to zero and set themselves as the proposer

• https://medium.com/immunefi/openzeppelin-bug-fix-postmortem-66d8c89ed166
• https://github.com/OpenZeppelin/openzeppelin-contracts/commit/cec4f2ef57495d8b1742d62846da212515d99dd5#diff-8229f9027848871a1706845a5a84fa3e6591445cfac6e16cfb7d652e91e8d395R307

2. In Governance.sol, there is an onlySelf modifier, which means that only the Governance contract itself can call. I find it rather interesting and had to understand how the contract can call its own function. Turns out, the function is called through an operation which calls execute() and subsequently calls the contract itself, so the msg.sender calling the contract is the Governance contract itself.

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

3. I learned about the beacon proxy creation when transferring ERC20 tokens from L1-L2. A new proxy contract is created for any amount of token bridged.

File: L2ERC20Bridge.sol
90:     /// @dev Deploy and initialize the L2 token for the L1 counterpart
91:     function _deployL2Token(address _l1Token, bytes calldata _data) internal returns (address) {
92:         bytes32 salt = _getCreate2Salt(_l1Token);
93:
94:         BeaconProxy l2Token = _deployBeaconProxy(salt);
95:         L2StandardERC20(address(l2Token)).bridgeInitialize(_l1Token, _data);
96:
97:         return address(l2Token);
98:     }

Time spent:

60 hours