search

code-423n4 / 2024-03-zksync-findings

1 stars 1 forks source link

An attacker could potentially claim another person's refund or apply for an excessive refund through the `claimFailedDepositLegacyErc20Bridge`. #36

Closed c4-bot-4 closed 4 months ago

c4-bot-4 commented 5 months ago

Lines of code

https://github.com/code-423n4/2024-03-zksync/blob/4f0ba34f34a864c354c7e8c47643ed8f4a250e13/code/contracts/ethereum/contracts/bridge/L1SharedBridge.sol#L329-L345 https://github.com/code-423n4/2024-03-zksync/blob/4f0ba34f34a864c354c7e8c47643ed8f4a250e13/code/contracts/ethereum/contracts/bridge/L1SharedBridge.sol#L644-L666

Vulnerability details

When initiating a refund request from the Legacy Bridge contract using claimFailedDepositLegacyErc20Bridge.

https://github.com/code-423n4/2024-03-zksync/blob/4f0ba34f34a864c354c7e8c47643ed8f4a250e13/code/contracts/ethereum/contracts/bridge/L1SharedBridge.sol#L644-L666

    function claimFailedDepositLegacyErc20Bridge(
        address _depositSender,
        address _l1Token,
        uint256 _amount,
        bytes32 _l2TxHash,
        uint256 _l2BatchNumber,
        uint256 _l2MessageIndex,
        uint16 _l2TxNumberInBatch,
        bytes32[] calldata _merkleProof
    ) external override onlyLegacyBridge {
        _claimFailedDeposit(
            true,
            ERA_CHAIN_ID,
            _depositSender,
            _l1Token,
            _amount,
            _l2TxHash,
            _l2BatchNumber,
            _l2MessageIndex,
            _l2TxNumberInBatch,
            _merkleProof
        );
    }

It calls the internal function _claimFailedDeposit.

https://github.com/code-423n4/2024-03-zksync/blob/4f0ba34f34a864c354c7e8c47643ed8f4a250e13/code/contracts/ethereum/contracts/bridge/L1SharedBridge.sol#L329-L345

        {
            bool notCheckedInLegacyBridgeOrWeCanCheckDeposit;
            {
                // Deposits that happened before the upgrade cannot be checked here, they have to be claimed and checked in the legacyBridge
                bool weCanCheckDepositHere = !_isEraLegacyWithdrawal(_chainId, _l2BatchNumber);
                // Double claims are not possible, as we this check except for legacy bridge withdrawals
                // Funds claimed before the update will still be recorded in the legacy bridge
                // Note we double check NEW deposits if they are called from the legacy bridge 
                notCheckedInLegacyBridgeOrWeCanCheckDeposit = (!_checkedInLegacyBridge) || weCanCheckDepositHere; 
            }
            if (notCheckedInLegacyBridgeOrWeCanCheckDeposit) {
                bytes32 dataHash = depositHappened[_chainId][_l2TxHash];
                bytes32 txDataHash = keccak256(abi.encode(_depositSender, _l1Token, _amount));
                require(dataHash == txDataHash, "ShB: d.it not hap");
                delete depositHappened[_chainId][_l2TxHash];
            }
        }

Impact

Proof of Concept

Attack scenario:

  1. Alice deposits 50 tokens on L1 before the Legacy Bridge update to the Shared Bridge.
  2. The deposit fails to roll up to L2 for some reason.
  3. Alice does not immediately request a refund from L1.
  4. The system is updated to the Shared Bridge.
  5. Alice requests a refund of 55 tokens from Legacy Bridge calling claimFailedDepositLegacyErc20Bridge.
  6. Since the deposit details (who initiated the deposit, what token was deposited, and the amount of the deposit) are not verified,
  7. Alice gets an excessive refund.

Test code

Added the test_excessiveRefundAttack function in L1ShardBridgeLegacy.t.sol

    function test_excessiveRefundAttack() public {
        // mint 100 for shareBridge，50 from alice, 50 from others
        token.mint(address(sharedBridge), amount);

        // storing depoistHappend[chainId][l2TxHash] = txDataHash. DepositHappened is 3rd so 3 -1 + dependency storage slots
        uint256 depositLocationInStorage = uint256(3 - 1 + 1 + 1);
        uint256 amountAlice = 50;
        uint256 amountAliceExcessive = 55;
        bytes32 txDataHash = keccak256(abi.encode(alice, address(token), amountAlice));
        vm.store(
            address(sharedBridge),
            keccak256(abi.encode(txHash, keccak256(abi.encode(ERA_CHAIN_ID, depositLocationInStorage)))),
            txDataHash
        );
        require(sharedBridge.depositHappened(ERA_CHAIN_ID, txHash) == txDataHash, "Deposit not set");

        uint256 chainBalanceLocationInStorage = uint256(6 - 1 + 1 + 1);
        vm.store(
            address(sharedBridge),
            keccak256(
                abi.encode(
                    uint256(uint160(address(token))),
                    keccak256(abi.encode(ERA_CHAIN_ID, chainBalanceLocationInStorage))
                )
            ),
            bytes32(amount)
        );

        // Bridgehub bridgehub = new Bridgehub();
        // vm.store(address(bridgehub),  bytes32(uint256(5 +2)), bytes32(uint256(31337)));
        // require(address(bridgehub.deployer()) == address(31337), "Bridgehub: deployer wrong");

        vm.mockCall(
            bridgehubAddress,
            abi.encodeWithSelector(
                IBridgehub.proveL1ToL2TransactionStatus.selector,
                ERA_CHAIN_ID,
                txHash,
                l2BatchNumber,
                l2MessageIndex,
                l2TxNumberInBatch,
                merkleProof,
                TxStatus.Failure
            ),
            abi.encode(true)
        );
        console2.log("Alice Before Refound: ", token.balanceOf(alice));

        vm.prank(l1ERC20BridgeAddress);
        sharedBridge.claimFailedDepositLegacyErc20Bridge(
            alice,
            address(token),
            amountAliceExcessive,
            txHash,
            l2BatchNumber,
            l2MessageIndex,
            l2TxNumberInBatch,
            merkleProof
        );
        console2.log("Alice Excessive Refound: ", token.balanceOf(alice));
    }

You should get the following output:

[PASS] test_excessiveRefundAttack() (gas: 138031)
Logs:
  Alice Before Refound:  0
  Alice Excessive Refound:  55

Test result: ok. 1 passed; 0 failed; 0 skipped; finished in 2.74ms

In addition to this， If an attacker can get a genuinely failed L2 deposit transaction, he can initiate a refund.

Tools Used

Manual

Recommended Mitigation Steps

Enhanced Verification: Increase the verification of key information such as the depositor, amount, and token type when processing refund requests.

Assessed type

Invalid Validation

c4-sponsor commented 5 months ago

saxenism (sponsor) disputed

saxenism commented 5 months ago

We consider this issue invalid because the amount is read from the legacy L1ERC20 bridge for these legacy transactions ( or sometimes even double checked).

alex-ppg commented 4 months ago

The PoC bypasses the access control imposed by the L1SharedBridge::claimFailedDepositLegacyErc20Bridge function which ensures it is in turn invoked by the legacy bridge that validates the arguments the Warden specifies are unsanitized. As such, this exhibit is considered invalid given that the variables are properly sanitized by the legacy bridge and cannot be arbitrarily supplied to the function referenced above.

c4-judge commented 4 months ago

alex-ppg marked the issue as unsatisfactory: Invalid