c4-submissions commented 1 year ago

Lines of code

https://github.com/code-423n4/2023-10-zksync/blob/1fb4649b612fac7b4ee613df6f6b7d921ddd6b0d/code/contracts/ethereum/contracts/zksync/facets/Mailbox.sol#L236-L273

Vulnerability details

Impact

Detailed description of the impact of this finding. The flow bridgeProxy.deposit() -> L1WethBridge.deposit() -> Mailbox.requestL2Transaction() allows one to deposit WETH from L1 to L2. However, when checking the deposit limit for a particular depositor, Mailbox.requestL2Transaction() checks the limit of msg.sender, which is the address of L1WethBridge. As a result, when the limit of the bridge is reached, nobody can deposit anymore, even though their limit is not reached yet. As a result, sooner or later, nobody will be able to use Zksync to deposit weth to L2 from L1.

Proof of Concept

Provide direct links to all referenced code in GitHub. Add screenshots, logs, or any other relevant proof that illustrates the concept.

The flow bridgeProxy.deposit() -> L1WethBridge.deposit() -> Mailbox.requestL2Transaction() allows one to deposit WETH from L1 to L2. However, when checking the deposit limit for a particular depositor, Mailbox.requestL2Transaction() checks the limit of msg.sender, which is the address of L1WethBridge.

https://github.com/code-423n4/2023-10-zksync/blob/1fb4649b612fac7b4ee613df6f6b7d921ddd6b0d/code/contracts/ethereum/contracts/zksync/facets/Mailbox.sol#L236-L273

As a result, when the limit of the bridge is reached, nobody can deposit anymore, even though their limit is not reached yet. As a result, sooner or later, nobody will be able to use Zksync to deposit weth to L2 from L1.

The following POC confirms my finding: 1) We set the deposit limit to 10 ether. 2) user1 deposits 3 ether of weth and sends 0.1 ether of eth, this is successful, we have s.totalDepositedAmountPerUser[L1WethBridge] = 3.1 ether. 3) user2 deposits 4 ether of weth and 0.1 ether of eth, this is successful, we have s.totalDepositedAmountPerUser[L1WethBridge] = 7.2 ether. 4) user3 deposits 2.7 ether of weth and 0.1 weth, this is successful s.totalDepositedAmountPerUser[L1WethBridge] = 10 ether, it has not exceeded the limit; however, if user3 deposits 2.7 ether + 1 of weth and 0.1 weth, then it will revert since now s.totalDepositedAmountPerUser[L1WethBridge] = 10 ether + 1. Note that user3 has not exceeded its limit. However, the limit check is on L1WethBridge, not on user3, and that is the problem.

The following test file is a modification of the test file: zksync/code/contracts/ethereum/test/foundry/unit/concrete/Bridge/L1WethBridge/Deposit.t.sol. The test function is test_DepositExceedLimit().

Please run the following command under zksync/code/contracts/ethereum:

forge test --match-test test_DepositExceedLimit -vv

The following line has been commented out to skip the check of merkle root:

https://github.com/code-423n4/2023-10-zksync/blob/1fb4649b612fac7b4ee613df6f6b7d921ddd6b0d/code/contracts/ethereum/contracts/zksync/facets/Mailbox.sol#L142

// SPDX-License-Identifier: MIT

pragma solidity ^0.8.17;

import "lib/forge-std/src/Test.sol";

import {L1WethBridgeTest} from "./_L1WethBridge_Shared.t.sol";
import {IAllowList} from "../../../../../../cache/solpp-generated-contracts/common/interfaces/IAllowList.sol";
import {REQUIRED_L2_GAS_PRICE_PER_PUBDATA} from "../../../../../../cache/solpp-generated-contracts/zksync/Config.sol";

contract DepositTest is L1WethBridgeTest {
    function deposit(address user, uint256 amount) private returns (bool) {
        hoax(user);
        l1Weth.deposit{value: amount}();

        hoax(user);
        l1Weth.approve(address(bridgeProxy), amount);

        bytes memory depositCallData = abi.encodeWithSelector(
            bridgeProxy.deposit.selector,
            user,
            bridgeProxy.l1WethAddress(),
            amount,
            1000000,                        // gas limit
            REQUIRED_L2_GAS_PRICE_PER_PUBDATA,
            user
        );

        hoax(user);
        (bool success, ) = address(bridgeProxy).call{value: 0.1 ether}(depositCallData); 
        return success;
    }

    function test_DepositExceedLimit() public {
        console.log("\n \n test_DepositExceeLimit is started....$$$$$$$$$$$$$$4");

        address user1 = address(111);
        address user2 = address(222);
        address user3 = address(333);

        vm.prank(owner);
        allowList.setDepositLimit(address(0), true, 10 ether); // deposit at most 10 ether
        IAllowList.Deposit memory limitData = IAllowList(allowList).getTokenDepositLimitData(address(0)); 
        assertEq(limitData.depositCap, 10 ether);

        bool success = deposit(user1, 3 ether); // send 3 ether weth and 0.1 ether eth
        assertTrue(success);

        success = deposit(user2, 4 ether); // send 4 ether weth and 0.1 ether eth
        assertTrue(success);

        success =  deposit(user3, 2.7 ether + 1); // send 2.7 ether + 1 weth  and 0.1 ether eth, now a total of 10ether + 1, will it exceed?  
        assertFalse(success);   // s.totalDepositedAmountPerUser[L1WethBridge] = 10 ether + 1, it exceeds the limit of 10 ether
    }
}

Tools Used

Foundry, VScode

Recommended Mitigation Steps

Check the limit of the real depositor instead of the limit for the L1WethBridge.

Assessed type

Math

c4-pre-sort commented 1 year ago

bytes032 marked the issue as sufficient quality report

c4-pre-sort commented 1 year ago

bytes032 marked the issue as primary issue

c4-sponsor commented 1 year ago

miladpiri marked the issue as disagree with severity

miladpiri commented 1 year ago

Deposit limitation is a temperoary feature implemented to protect users from depositing large amount of fund into the protocol while it is in alpha mode. So, issues like this does not cause damage to the protocol. So, medium can be a fair severity.

c4-sponsor commented 1 year ago

miladpiri (sponsor) confirmed

c4-judge commented 1 year ago

GalloDaSballo changed the severity to 2 (Med Risk)

GalloDaSballo commented 1 year ago

The Warden has shown how the implementation of limits, which are indexed by address, will cause the bridge to be subject to the caps that should apply to a single wallet

Since the finding would deny functionality but can technically be removed, I think Medium Severity to be most appropriate