zkSync Era - zzzitron's results

Lines of code

https://github.com/code-423n4/2023-10-zksync/blob/615333f3c5ef3e1249879b56524214d4710c9fd7/code/contracts/ethereum/contracts/bridge/L1ERC20Bridge.sol#L188 https://github.com/code-423n4/2023-10-zksync/blob/615333f3c5ef3e1249879b56524214d4710c9fd7/code/contracts/ethereum/contracts/bridge/L1ERC20Bridge.sol#L278 https://github.com/code-423n4/2023-10-zksync/blob/615333f3c5ef3e1249879b56524214d4710c9fd7/code/contracts/ethereum/contracts/bridge/L1ERC20Bridge.sol#L340 https://github.com/code-423n4/2023-10-zksync/blob/615333f3c5ef3e1249879b56524214d4710c9fd7/code/contracts/ethereum/contracts/bridge/L1ERC20Bridge.sol#L342

Vulnerability details

Impact

HIGH - Assets can be locked

If the deposit limit is updated, users might not be able to claim their failed deposits.

For example, a user deposits while there is no deposit limit. Then the owner sets the deposit limit for that token. Then the issue may occur that the user may not be able to claim their failed deposit due to an underflow on line 345 in L1ERC20Bridge.sol.

Proof of Concept

Below is a snippet of the proof of concept demonstrating the issue. The full code can be found in this gist. The poc is written based on the code/contracts/ethereum/test/foundry/unit/concrete/Bridge/L1WethBridge/_L1WethBridge_Shared.t.sol

  function test_claimFailedDeposit_poc() public {
      uint256 amount = 10000;

      l1Weth.deposit{value: amount}();
      l1Weth.approve(address(bridgeProxy), amount);

      // user (address(this)) deposit
      bytes32 l2TxHash = bridgeProxy.deposit(randomSigner, address(l1Weth), amount, 1000000, REQUIRED_L2_GAS_PRICE_PER_PUBDATA, randomSigner);

      // owner setDepositLimit
      vm.prank(owner);
      allowList.setDepositLimit(address(l1Weth), true, type(uint256).max);
      
      // user (address(this)) claimFailedDeposit
      vm.expectRevert(stdError.arithmeticError);
      bridgeProxy.claimFailedDeposit(address(this), address(l1Weth), l2TxHash, 1, 2, 3, new bytes32[](0));
  }

1. user (the test contract) deposits via L1ERC20Bridge.deposit

• note that there is no deposit limit set for the depositted token

2. owner updates the deposit limit via AllowList.setDepositLimit
3. user (the test contract) attempts to claim failed deposit via L1ERC20Bridge.claimFailedDeposit, however fails with "Arithmetic over/underflow"

If the owner would have not updated the deposit limit, the user could have claimed the failed deposit.

Note: A simple mock zkSync contract is used for the ease of the test, which will return true for all proveL1ToL2TransactionStatus call.

Details

In the functions L1ERC20Bridge.deposit and L1ERC20Bridge.claimFailedDeposit, L1ERC20Bridge._verifyDepositLimit is called to update totalDepositedAmountPerUser.

https://github.com/code-423n4/2023-10-zksync/blob/615333f3c5ef3e1249879b56524214d4710c9fd7/code/contracts/ethereum/contracts/bridge/L1ERC20Bridge.sol#L188 https://github.com/code-423n4/2023-10-zksync/blob/615333f3c5ef3e1249879b56524214d4710c9fd7/code/contracts/ethereum/contracts/bridge/L1ERC20Bridge.sol#L278 https://github.com/code-423n4/2023-10-zksync/blob/615333f3c5ef3e1249879b56524214d4710c9fd7/code/contracts/ethereum/contracts/bridge/L1ERC20Bridge.sol#L340

However, the totalDepositedAmountPerUser is only updated if the depositLimitation is set to be true.

https://github.com/code-423n4/2023-10-zksync/blob/615333f3c5ef3e1249879b56524214d4710c9fd7/code/contracts/ethereum/contracts/bridge/L1ERC20Bridge.sol#L342

So, if the user deposits while the depositLimitation is not set (false), the user's totalDepositedAmountPerUser will be remain zero.

After the owner set the depositLimitation to be true, if the user should call the L1ERC20Bridge.claimFailedDeposit, it will attempt to reduce the totalDepositedAmountPerUser, which may result in Arithmetic over/underflow.

Tools Used

Manual

Recommended Mitigation Steps

Consider updating totalDepositedAmountPerUser even when depositLimitation is not set.

<!-- zzzitron H01 -->

Assessed type

Under/Overflow