User might be able to double withdraw during migration

[c4-bot-8]

Lines of code

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

Vulnerability details

Impact

A user might be able to double withdraw during migration in some edge conditions: (1) if their withdrawal tx is included in a batch number the same or after eraFirstPostUpgradeBatch; (2)And if the user finalizeWithdrawal on the old L1ERC20Bridge.sol before L1ERC20Bridge.sol is upgraded.

Proof of Concept

The current migration process takes place in steps, which might allow edge conditions to occur. StepA: deploy new contracts(including L1SharedBridge); StepB:Era upgrade and L2 system contracts upgrade(at this point old L1ERC20Bridge still works); StepC: Upgrade L2 bridge and L1ERC20Brdige.sol. Then migrate funds to the new L1sharedBridge.

Since L1ERC20Bridge.sol is upgraded at the end. An edge condition can occur between StepA and StepB, where a user can still withdraw ERC20 tokens on the old L1ERC20Brdige.sol.

Scenario: If a user finalizeWithdrawal ERC20 tokens on the old L1ERC20Bridge.sol first, they can withdraw again on L1SharedBridge.sol as long as the _l2batchNumber of the withdraw tx equals or is greater than eraFirstPostUpgradeBatch. In other words, _isEraLegacyWithdrawal check can be invalidated.

1. stepA: L1SharedBridge.sol will be deployed and initialized with a pre-determined value eraFirstPostUpgradeBatch;
2. UserA can initiate an ERC20 withdrawal tx on L2 bridge(currently old version);
3. stepB: Era upgrade is performed. UserA's withdrawal tx is included in the same eraFirstPostUpgradeBatch number;
4. UserA finializeWithdrawal on the old L1ERC20Brdige.sol.UserA should receive funds because funds haven't been migrated yet;
5. stepC: new L1ERC20Bridge.sol is upgraded and funds(ERC20) are migrated to L1SharedBridge.sol;
6. UserA finializeWithdrawal on L1SharedBridge.sol. This time, _isEraLegacyWithdrawal(_chainId, _l2BatchNumber) check will be bypassed,because user's _l2BatchNumber == eraFirstPostUpgradeBatch; UserA can receive the withdrawal funds a second time;

   //code/contracts/ethereum/contracts/bridge/L1SharedBridge.sol
   function finalizeWithdrawal(
       uint256 _chainId,
       uint256 _l2BatchNumber,
       uint256 _l2MessageIndex,
       uint16 _l2TxNumberInBatch,
       bytes calldata _message,
       bytes32[] calldata _merkleProof
   ) external override {
      //@audit-info when _l2BatchNumber>= eraFirstPostUpgradeBatch, `_isEraLegacyWithdrawal()` return false, checking on withdrawal status on legacyERC20bridge will be bypassed.
   |>     if (_isEraLegacyWithdrawal(_chainId, _l2BatchNumber)) {
           require(
               !legacyBridge.isWithdrawalFinalized(
                   _l2BatchNumber,
                   _l2MessageIndex
               ),
               "ShB: legacy withdrawal"
           );
       }
       _finalizeWithdrawal(
           _chainId,
           _l2BatchNumber,
           _l2MessageIndex,
           _l2TxNumberInBatch,
           _message,
           _merkleProof
       );
   }

   (https://github.com/code-423n4/2024-03-zksync/blob/4f0ba34f34a864c354c7e8c47643ed8f4a250e13/code/contracts/ethereum/contracts/bridge/L1SharedBridge.sol#L421-L427)

   //code/contracts/ethereum/contracts/bridge/L1SharedBridge.sol
   function _isEraLegacyWithdrawal(
       uint256 _chainId,
       uint256 _l2BatchNumber
   ) internal view returns (bool) {
       return
           (_chainId == ERA_CHAIN_ID) &&
   |>          (_l2BatchNumber < eraFirstPostUpgradeBatch); //@audit-info note:when _l2BatchNumber>= eraFirstPostUpgradeBatch, `_isEraLegacyWithdrawal()` return false.
   }

   (https://github.com/code-423n4/2024-03-zksync/blob/4f0ba34f34a864c354c7e8c47643ed8f4a250e13/code/contracts/ethereum/contracts/bridge/L1SharedBridge.sol#L375) As seen, checking on legacyERC20bridge withdrawal status will only be performed when _isEraLegacyWithdrawal returns true. But due to _l2BatchNumber of a withdrawal tx during the upgrade can equal or be greater than a predefined eraFirstPostUpgradeBatch. _isEraLegacyWithdrawal check can be based on false assumptions on _l2BatchNumber and eraFirstPostUpgradeBatch, allowing double withdrawal on edge cases.

Tools Used

Manual

Recommended Mitigation Steps

Consider adding a grace period during and following an upgrade, during which time legacyWithdrawal status will always be checked.

Assessed type

Other

[c4-sponsor]

razzorsec marked the issue as disagree with severity

[c4-sponsor]

razzorsec (sponsor) confirmed

[razzorsec]

Relistically invalid, since we will not finalize the upgrade batch. But an interesting thing to remember, so we would consider this as Low, as it is mostly about managing the server.

[alex-ppg]

The Warden specifies that under certain circumstances, it is possible to perform a duplicate withdrawal when the system is in the midst of an upgrade.

The Sponsor specifies that this issue is realistically invalid, however, the Sponsor's statement relies on being aware of the flaw and acting actively against it (i.e. not finalizing the upgrade batch) which I do not consider adequate justification to lower the severity of this exhibit.

I believe a medium-risk grade is appropriate based on the fact that it illustrates a code flaw that will arise under operations permitted by the smart contracts which we cannot presume the Sponsor was aware of.

[c4-judge]

alex-ppg marked the issue as satisfactory

[c4-judge]

alex-ppg marked the issue as selected for report