Uninitializing Bridge Contracts' State Variables

[code423n4]

Lines of code

https://github.com/code-423n4/2022-10-zksync/blob/main/ethereum/contracts/bridge/L1ERC20Bridge.sol#L57-L60 https://github.com/code-423n4/2022-10-zksync/blob/main/ethereum/contracts/bridge/L1EthBridge.sol#L48-L51

Vulnerability details

Vulnerability Details

The L1ERC20Bridge and L1EthBridge are implementation contracts that would be delegatecalled by their corresponding proxy contracts. In other words, all state variables and assets would be stored in the proxy contracts. In contrast, the implementation contracts would keep the logic for processing the state variables stored in the proxy contracts.

We found that the L1ERC20Bridge and L1EthBridge contracts initialize their state variables, including zkSyncMailbox and allowList, in the constructor (L57 - 60 in code snippet 1 and L48 - 51 in code snippet 2).

Specifically, initializing state variables in the implementation contracts' constructor would not be effective on the proxy contracts. Consequently, the resulting uninitialized state variables (i.e., zkSyncMailbox and allowList) can render the proxy contracts unusable.

SNIPPET: 1
FILE: https://github.com/code-423n4/2022-10-zksync/blob/main/ethereum/contracts/bridge/L1ERC20Bridge.sol

21:     contract L1ERC20Bridge is IL1Bridge, AllowListed, ReentrancyGuard {
            // (...SNIPPED...)

24:         /// @dev The smart contract that manages the list with permission to call contract functions
25:         IAllowList immutable allowList;
26: 
27:         /// @dev zkSync smart contract that used to operate with L2 via asynchronous L2 <-> L1 communication
28:         IMailbox immutable zkSyncMailbox;

            // (...SNIPPED...)

57:         constructor(IMailbox _mailbox, IAllowList _allowList) reentrancyGuardInitializer {
58:             zkSyncMailbox = _mailbox;
59:             allowList = _allowList;
60:         }

            // (...SNIPPED...)
288:    }

SNIPPET: 2
FILE: https://github.com/code-423n4/2022-10-zksync/blob/main/ethereum/contracts/bridge/L1EthBridge.sol

17:     contract L1EthBridge is IL1Bridge, AllowListed, ReentrancyGuard {
18:         /// @dev The smart contract that manages the list with permission to call contract functions
19:         IAllowList immutable allowList;
20: 
21:         /// @dev zkSync smart contract used to interact with L2 via asynchronous L2 <-> L1 communication
22:         IMailbox immutable zkSyncMailbox;

            // (...SNIPPED...)

48:         constructor(IMailbox _mailbox, IAllowList _allowList) reentrancyGuardInitializer {
49:             zkSyncMailbox = _mailbox;
50:             allowList = _allowList;
51:         }

            // (...SNIPPED...)
246:     }

Impact

We found that the L1ERC20Bridge and L1EthBridge contracts initialize their state variables, including zkSyncMailbox and allowList, in the constructor (L57 - 60 in code snippet 1 and L48 - 51 in code snippet 2).

Specifically, initializing state variables in the implementation contracts' constructor would not be effective on the proxy contracts. Consequently, the resulting uninitialized state variables (i.e., zkSyncMailbox and allowList) can render the proxy contracts unusable.

For this reason, we considered this issue high severity.

Proof of Concept

https://github.com/code-423n4/2022-10-zksync/blob/main/ethereum/contracts/bridge/L1ERC20Bridge.sol#L57-L60

https://github.com/code-423n4/2022-10-zksync/blob/main/ethereum/contracts/bridge/L1EthBridge.sol#L48-L51

Tools Used

VSCode (Manual Review)

Recommended Mitigation Steps

We recommend initializing the state variables zkSyncMailbox and allowList in the initialize function, like L75 - 76 in code snippet 3 and L58 - 59 in code snippet 4.

The initialize function would be delegatecalled by the proxy contracts during the system initialization process. Therefore, the state variables zkSyncMailbox and allowList would be initialized and effective on the proxy contracts.

SNIPPET: 3
FILE: https://github.com/code-423n4/2022-10-zksync/blob/main/ethereum/contracts/bridge/L1ERC20Bridge.sol

21:     contract L1ERC20Bridge is IL1Bridge, AllowListed, ReentrancyGuard {
            // (...SNIPPED...)

24:         /// @dev The smart contract that manages the list with permission to call contract functions
25:         IAllowList public allowList;
26: 
27:         /// @dev zkSync smart contract that used to operate with L2 via asynchronous L2 <-> L1 communication
28:         IMailbox public zkSyncMailbox;

            // (...SNIPPED...)

57:         constructor() reentrancyGuardInitializer {}

            // (...SNIPPED...)

68:         function initialize(
69:             IMailbox _mailbox, 
70:             IAllowList _allowList,
71:             bytes[] calldata _factoryDeps,
72:             address _l2TokenFactory,
73:             address _governor
74:         ) external reentrancyGuardInitializer {
75:             zkSyncMailbox = _mailbox;
76:             allowList = _allowList;

                // (...SNIPPED...)

104:        }

            // (...SNIPPED...)
292:    }

SNIPPET: 4
FILE: https://github.com/code-423n4/2022-10-zksync/blob/main/ethereum/contracts/bridge/L1EthBridge.sol

17:     contract L1EthBridge is IL1Bridge, AllowListed, ReentrancyGuard {
18:         /// @dev The smart contract that manages the list with permission to call contract functions
19:         IAllowList public allowList;
20: 
21:         /// @dev zkSync smart contract used to interact with L2 via asynchronous L2 <-> L1 communication
22:         IMailbox public zkSyncMailbox;

            // (...SNIPPED...)

48:         constructor() reentrancyGuardInitializer {}

            // (...SNIPPED...)

53:         function initialize(
54:             IMailbox _mailbox, 
55:             IAllowList _allowList, 
56:             bytes calldata _l2BridgeBytecode
57:         ) external reentrancyGuardInitializer {
58:             zkSyncMailbox = _mailbox;
59:             allowList = _allowList;

                // (...SNIPPED...)
84:         }

            // (...SNIPPED...)
250:    }

[GalloDaSballo]

Looks off without showing a POC of the variables not being set properly.

Immutable variables will be set at deploy time and will alter the bytecode of the contract deployed

[GalloDaSballo]

This is incorrect, the way it's going to work is as follows:

• Logic -> Deployed -> Immutable variables are set in the bytecode
• Proxy -> Maps onto the Logic, uses the immutable variables as those are from the bytecode of the Logic.

In lack of this being acknowledged, and then disputed, I will close as invalid (Incorrect)

[c4-judge]

GalloDaSballo marked the issue as unsatisfactory: Invalid