zkSync Era - brgltd's results

Lines of code

https://github.com/code-423n4/2023-10-zksync/blob/main/code/contracts/ethereum/contracts/bridge/L1ERC20Bridge.sol#L278 https://github.com/code-423n4/2023-10-zksync/blob/main/code/contracts/ethereum/contracts/bridge/L1ERC20Bridge.sol#L345 https://github.com/code-423n4/2023-10-zksync/blob/main/code/contracts/ethereum/contracts/common/AllowList.sol#L129-L132

Vulnerability details

Proof of Concept

• User calls L1ERC20Bridge.deposit() to bridge some some L1 token, which will call L1ERC20Bridge._verifyDepositLimit() internally.

• Assume depositLimitation is set to false. This will result in totalDepositedAmountPerUser[_l1Token][_depositor] not being updated due to the early termination inside L1ERC20Bridge._verifyDepositLimit(). Let's assume that depositLimitation was always false up to this point and therefore totalDepositedAmountPerUser[_l1Token][_depositor] will still have the default value of zero, since it's nested mapping for a uint256

• Assume the finalization fails on L2 and and the user wants to claim his funds

• Before the user claims, the owner calls AllowList.setDepositLimit(), this time to set depositLimitation to true and passing some value for depositCap

• The user tries to claim by calling L1ERC20Bridge.claimFailedDeposit() which will call L1ERC20Bridge._verifyDepositLimit() again.

• Computing the value for totalDepositedAmountPerUser will underflow, since the current value for this variable is zero, e.g. it will try to decrement zero by some amount, reverting the tx.

Coded Proof of Concept

To create a full coded proof of concept, we would need to force a tx to fail on L2 - to make the merkle proof verification pass with status/value TxStatus.Failure.

I could not find a test case for L1ERC20.claimFailedDeposit() on the hardhat or foundry testing setup. I'm also not sure how to force the tx to fail on L2.

Therefore, I'm commenting the merkle proof verification - L264-L272. This way, I can prove that if the owner changes depositLimitation, the user claim will fail with a revert.

The following test case can be added in l1_erc20_bridge_test.spec.ts and then executed with yarn run test. As mentioned above, L264-L272 from the contract source code needs to be commented in order to work. Note I'm calling allowList.setDepositLimit() before the user claims, which causes the underflow.

it(`panic/underflow inside L1ERC20Bridge.claimFailedDeposit()`, async () => {
    const depositorAddress = await randomSigner.getAddress();
    const l1Token = testnetERC20TokenContract.address;

    // Admin sets public access for the bridge and no deposit limit for l1Token.
    await (await allowList.setAccessMode(l1Erc20BridgeContract.address, AccessMode.Public)).wait();
    await (await allowList.setDepositLimit(l1Token, false, 0)).wait();

    // Deposit and check event with value for l2TxHash - since this hash will be used to claim the failed deposit.
    const expectedL2TxHash = "0xeeca29d93132357d5dc2a6100a0b3492fd61c587235b0ae70e139ca406f82958";
    await expect(
        depositERC20(
            l1ERC20Bridge.connect(randomSigner), // bridge
            zksyncContract, // zksyncContract
            depositorAddress, // l2Receiver
            l1Token, // l1Token
            ethers.utils.parseUnits('800', 18), // amount
            10000000 // l2GasLimit
        ),
    )
    .to.emit(l1Erc20BridgeContract, "DepositInitiated")
    .withArgs(
        expectedL2TxHash, // l2TxHash
        depositorAddress, // msg.sender
        depositorAddress, // _l2Receiver
        l1Token, // l1Token
        ethers.utils.parseUnits('800', 18) // amount
    );

    // Assuming finalization failed on L2...

    // Admin sets depositLimitation to true and 1000e18 as depositCap for the l1Token.
    // If we comment the line below (pretend depositLimit didn't change), then calling claimFailedDeposit() would not revert.
    await (await allowList.setDepositLimit(l1Token, true, ethers.utils.parseUnits("1000", 18))).wait();

    // Claim the failed deposit will revert due to underflow.
    // Note I'm commenting L268 to L276 in L1ERC20Bridge and passing mocked values for _l2BatchNumber, _l2MessageIndex, _l2TxNumberInBatch and _merkleProof
    // Otherwise, we need to simulate the tx failing on L2, and pass the l2 data alongside the _merkleProof with TxStatus.failure.
    const revertReason = await getCallRevertReason(
        l1ERC20Bridge.connect(randomSigner).claimFailedDeposit(
            depositorAddress, // _depositSender
            l1Token, // _l1Token 
            expectedL2TxHash, // _l2TxHash
            0, // _l2BatchNumber, mocked 
            0, // _l2MessageIndex, mocked
            0, // _l2TxNumberInBatch, mocked
            [], // _merkleProof, mocked
        ),
    );
    expect(revertReason).equal("VM Exception while processing transaction: reverted with panic code 0x11 (Arithmetic operation underflowed or overflowed outside of an unchecked block)");
});

Impact

Even if setting a deposit limitation for L1 tokens is an onboarding feature and even if the owner can change the value later to allow claiming the failed deposit, I would argue that a user should be able to claim his failed deposit at all times, without the need to intervention from the owner.

Changing the depositLimitation to cause the issue mentioned on this report could happen accidentally or in the worst case maliciously. Nobody would assume zkSync would proceed with such actions, but currently users would have to trust that zkSync won't do that. I would argue that for blockchain system to truly succeed against traditional banking, trustless and permissionless features need to taken very seriously, specially when dealing with tokens/funds.

What I'm arguing is that even is this backdoor was completely accidental, it should not be present in the system.

Tools Used

Manual review

Recommended Mitigation Steps

One solution could be store the active depositLimitation and depositCap for each tx when calling L1ERC20Bridge.deposit(), e.g. save these values for each _l2TxHash. This would allow using the depositLimitation and depositCap values that were active during the deposit when calling L1ERC20Bridge.claimFailedDeposit().

Another solution could be to check if totalDepositedAmountPerUser[_l1Token][_depositor] is not zero inside L1ERC20Bridge._verifyDepositLimit() when calling from L1ERC20Bridge.claimFailedDeposit() inside the conditional on L345.

Assessed type

Rug-Pull