code423n4 commented 1 year ago

Lines of code

https://github.com/code-423n4/2022-10-zksync/blob/main/ethereum/contracts/zksync/libraries/Diamond.sol#L137 https://github.com/code-423n4/2022-10-zksync/blob/main/ethereum/contracts/zksync/libraries/Diamond.sol#L161 https://github.com/code-423n4/2022-10-zksync/blob/main/ethereum/contracts/zksync/libraries/Diamond.sol#L200-L223 https://github.com/code-423n4/2022-10-zksync/blob/main/ethereum/contracts/zksync/facets/DiamondCut.sol#L83 https://github.com/code-423n4/2022-10-zksync/blob/main/ethereum/contracts/zksync/facets/DiamondCut.sol#L98 https://github.com/code-423n4/2022-10-zksync/blob/main/ethereum/contracts/zksync/DiamondProxy.sol#L30

Vulnerability details

Vulnerability Details

The DiamondCutFacet is a special facet contract for managing the freezing, unfreezing, and upgrading of other facet contracts.

We discovered a critical issue regarding the diamond freeze feature, which allows a governor to freeze the Diamond contract. If the DiamondCutFacet was set to be a freezable facet by mistake, the Diamond contract would be frozen unrecoverably, affecting all platform's assets and users' assets on both L1 and L2 networks.

The root cause of this issue resides in the function _addOneFunction of the Diamond contract (code snippet 1). We found that the _addOneFunction function allows a governor to register the DiamondCutFacet contract as a freezable facet by mistake.

In other words, the _addOneFunction function lacks a proper verification process when the DiamondCutFacet contract is registered.

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

200:    function _addOneFunction(
201:        address _facet,
202:        bytes4 _selector,
203:        bool _isSelectorFreezable
204:    ) private {
205:        DiamondStorage storage ds = getDiamondStorage();
206:
207:        uint16 selectorPosition = uint16(ds.facetToSelectors[_facet].selectors.length);
208:
209:        // if selectorPosition is nonzero, it means it is not a new facet
210:        // so the freezability of the first selector must be matched to _isSelectorFreezable
211:        // so all the selectors in a facet will have the same freezability
212:        if (selectorPosition != 0) {
213:            bytes4 selector0 = ds.facetToSelectors[_facet].selectors[0];
214:            require(_isSelectorFreezable == ds.selectorToFacet[selector0].isFreezable, "J1");
215:        }
216:
217:        ds.selectorToFacet[_selector] = SelectorToFacet({
218:            facetAddress: _facet,
219:            selectorPosition: selectorPosition,
220:            isFreezable: _isSelectorFreezable
221:        });
222:        ds.facetToSelectors[_facet].selectors.push(_selector);
223:    }

Two diamond cut actions that can cause this issue include Add and Replace. Specifically, the Add action would trigger the _addFunctions function (L119 - 139 in code snippet 2), whereas the Replace action would trigger the _replaceFunctions function (L143 - 163).

We found that both functions would invoke the _addOneFunction function in L137 and L161 to trigger the issue.

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

117:    /// @dev Add new functions to the diamond proxy
118:    /// NOTE: expect but NOT enforce that `_selectors` is NON-EMPTY array
119:    function _addFunctions(
120:        address _facet,
121:        bytes4[] memory _selectors,
122:        bool _isFacetFreezable
123:    ) private {
124:        DiamondStorage storage ds = getDiamondStorage();
125:
126:        require(_facet != address(0), "G"); // facet with zero address cannot be added
127:
128:        // Add facet to the list of facets if the facet address is new one
129:        _saveFacetIfNew(_facet);
130:
131:        uint256 selectorsLength = _selectors.length;
132:        for (uint256 i = 0; i < selectorsLength; ++i) {
133:            bytes4 selector = _selectors[i];
134:            SelectorToFacet memory oldFacet = ds.selectorToFacet[selector];
135:            require(oldFacet.facetAddress == address(0), "J"); // facet for this selector already exists
136:
137:            _addOneFunction(_facet, selector, _isFacetFreezable);
138:        }
139:    }
140:
141:    /// @dev Change associated facets to already known function selectors
142:    /// NOTE: expect but NOT enforce that `_selectors` is NON-EMPTY array
143:    function _replaceFunctions(
144:        address _facet,
145:        bytes4[] memory _selectors,
146:        bool _isFacetFreezable
147:    ) private {
148:        DiamondStorage storage ds = getDiamondStorage();
149:
150:        require(_facet != address(0), "K"); // cannot replace facet with zero address
151:
152:        uint256 selectorsLength = _selectors.length;
153:        for (uint256 i = 0; i < selectorsLength; ++i) {
154:            bytes4 selector = _selectors[i];
155:            SelectorToFacet memory oldFacet = ds.selectorToFacet[selector];
156:            require(oldFacet.facetAddress != address(0), "L"); // it is impossible to replace the facet with zero address
157:
158:            _removeOneFunction(oldFacet.facetAddress, selector);
159:            // Add facet to the list of facets if the facet address is a new one
160:            _saveFacetIfNew(_facet);
161:            _addOneFunction(_facet, selector, _isFacetFreezable);
162:        }
163:    }

Code snippet 3 below presents the functions emergencyFreezeDiamond (L78 - 88) and unfreezeDiamond (L91 - 101).

The emergencyFreezeDiamond function allows a governor to pause all freezable facets' functions instantly (L83). If the DiamondCutFacet contract was set to be a freezable facet, its functions would also be frozen.

From our investigation, if the DiamondCutFacet contract is frozen, a governor would have no way to unfreeze the contract because the execution of the unfreezeDiamond function to unfreeze the Diamond contract (L98) would not be reachable any longer.

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

77:     /// @notice Instantly pause the functionality of all freezable facets & their selectors
78:     function emergencyFreezeDiamond() external onlyGovernor {
79:         Diamond.DiamondStorage storage diamondStorage = Diamond.getDiamondStorage();
80:         require(!diamondStorage.isFrozen, "a9"); // diamond proxy is frozen already
81:         _resetProposal();
82: 
83:         diamondStorage.isFrozen = true;
84:         // Limited-time freezing feature will be added in the future upgrades, so keeping this variable for simplification
85:         s.diamondCutStorage.lastDiamondFreezeTimestamp = block.timestamp;
86:
87:         emit EmergencyFreeze();
88:     }
89: 
90:     /// @notice Unpause the functionality of all freezable facets & their selectors
91:     function unfreezeDiamond() external onlyGovernor {
92:         Diamond.DiamondStorage storage diamondStorage = Diamond.getDiamondStorage();
93: 
94:         require(diamondStorage.isFrozen, "a7"); // diamond proxy is not frozen
95: 
96:         _resetProposal();
97: 
98:         diamondStorage.isFrozen = false;
99: 
100:        emit Unfreeze(s.diamondCutStorage.lastDiamondFreezeTimestamp);
101:    }

To elaborate on the case when the DiamondCutFacet contract is frozen, the DiamondProxy's fallback function would reject the execution of the DiamondCutFacet's unfreezeDiamond function in L30 in the code snippet 4 below.

Therefore, this issue could impact all platform's assets and users' assets on both L1 and L2 networks.

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

19:     fallback() external payable {
20:         Diamond.DiamondStorage storage diamondStorage = Diamond.getDiamondStorage();
21:         // Check whether the data contains a "full" selector or it is empty.
22:         // Required because Diamond proxy finds a facet by function signature,
23:         // which is not defined for data length in range [1, 3].
24:         require(msg.data.length >= 4 || msg.data.length == 0, "Ut");
25:         // Get facet from function selector
26:         Diamond.SelectorToFacet memory facet = diamondStorage.selectorToFacet[msg.sig];
27:         address facetAddress = facet.facetAddress;
28: 
29:         require(facetAddress != address(0), "F"); // Proxy has no facet for this selector
30:         require(!diamondStorage.isFrozen || !facet.isFreezable, "q1"); // Facet is frozen
31: 
32:         assembly {
33:             // The pointer to the free memory slot
34:             let ptr := mload(0x40)
35:             // Copy function signature and arguments from calldata at zero position into memory at pointer position
36:             calldatacopy(ptr, 0, calldatasize())
37:             // Delegatecall method of the implementation contract returns 0 on error
38:             let result := delegatecall(gas(), facetAddress, ptr, calldatasize(), 0, 0)
39:             // Get the size of the last return data
40:             let size := returndatasize()
41:             // Copy the size length of bytes from return data at zero position to pointer position
42:             returndatacopy(ptr, 0, size)
43:             // Depending on the result value
44:             switch result
45:             case 0 {
46:                 // End execution and revert state changes
47:                 revert(ptr, size)
48:             }
49:             default {
50:                 // Return data with length of size at pointers position
51:                 return(ptr, size)
52:             }
53:         }
54:     }

Impact

We discovered a critical issue regarding the diamond freeze feature, which allows a governor to freeze the Diamond contract. If the DiamondCutFacet was set to be a freezable facet by mistake, the Diamond contract would be frozen unrecoverably, affecting all platform's assets and users' assets on both L1 and L2 networks.

Proof of Concept

https://github.com/code-423n4/2022-10-zksync/blob/main/ethereum/contracts/zksync/libraries/Diamond.sol#L137

https://github.com/code-423n4/2022-10-zksync/blob/main/ethereum/contracts/zksync/libraries/Diamond.sol#L161

https://github.com/code-423n4/2022-10-zksync/blob/main/ethereum/contracts/zksync/libraries/Diamond.sol#L200-L223

https://github.com/code-423n4/2022-10-zksync/blob/main/ethereum/contracts/zksync/facets/DiamondCut.sol#L83

https://github.com/code-423n4/2022-10-zksync/blob/main/ethereum/contracts/zksync/facets/DiamondCut.sol#L98

https://github.com/code-423n4/2022-10-zksync/blob/main/ethereum/contracts/zksync/DiamondProxy.sol#L30

Tools Used

VSCode (Manual Review)

Recommended Mitigation Steps

We recommend implementing a proper facet verification mechanism by ensuring that the DiamondCutFacet contract cannot be set as a freezable facet as follows.

Code snippet 5 shows the DiamondCutFacet contract. We implemented the pure function isDiamondCutFacet (L138 - 140) that would return true of type bool. This function would be used in the verification process, which will be explained later.

SNIPPET: 5
FILE: https://github.com/code-423n4/2022-10-zksync/blob/main/ethereum/contracts/zksync/facets/DiamondCut.sol

12:     contract DiamondCutFacet is Base, IDiamondCut {
            // (...SNIPPED...)

138:        function isDiamondCutFacet() external pure virtual override returns (bool) {
139:            return true;
140:        }
141:    }

In the code snippet 6 below, we defined the isDiamondCutFacet function (L37) in the IDiamondCut interface.

SNIPPET: 6
FILE: https://github.com/code-423n4/2022-10-zksync/blob/main/ethereum/contracts/zksync/interfaces/IDiamondCut.sol

7:      interface IDiamondCut {
            // (...SNIPPED...)

37:         function isDiamondCutFacet() external pure returns (bool);
38:     }

The _addOneFunction function was updated to implement the proposed facet verification mechanism as presented in L216 - 237 in the code snippet 7 below.

In case a new facet is registered to the system, the _addOneFunction function would verify that the new facet must not be the freezable DiamondCutFacet contract.

To verify the new facet, specifically, the _addOneFunction function would make a staticcall to the isDiamondCutFacet function of the target facet (L219 - 220).

If the target facet is the DiamondCutFacet contract, the isDiamondCutFacet function would return the value true making a transaction to be reverted in L223.

Otherwise, the verification process would be succeeded regardless of the call result. In other words, we would imply that the target facet would not be the DiamondCutFacet contract since the target facet does not implement the legitimate isDiamondCutFacet function.

SNIPPET: 7
FILE: https://github.com/code-423n4/2022-10-zksync/blob/main/ethereum/contracts/zksync/libraries/Diamond.sol

200:    function _addOneFunction(
201:        address _facet,
202:        bytes4 _selector,
203:        bool _isSelectorFreezable
204:    ) private {
205:        DiamondStorage storage ds = getDiamondStorage();
206:
207:        uint16 selectorPosition = uint16(ds.facetToSelectors[_facet].selectors.length);
208:
209:        // if selectorPosition is nonzero, it means it is not a new facet
210:        // so the freezability of the first selector must be matched to _isSelectorFreezable
211:        // so all the selectors in a facet will have the same freezability
212:        if (selectorPosition != 0) {
213:            bytes4 selector0 = ds.facetToSelectors[_facet].selectors[0];
214:            require(_isSelectorFreezable == ds.selectorToFacet[selector0].isFreezable, "J1");
215:        }
216:        else if (_isSelectorFreezable) {
217:            // If a new facet is meant to be a freezable facet, 
218:            // we have to verify that it is not the DiamondCut facet
219:            try DiamondCutFacetVerificationHelper.functionStaticCall(
220:                _facet, abi.encodeCall(IDiamondCut.isDiamondCutFacet, ())) returns (bytes memory returnedData) {
221:
222:                try DiamondCutFacetVerificationHelper.decodeBool(returnedData) returns (bool isDiamondCutFacet) {
223:                    require(!isDiamondCutFacet, "DiamondCut cannot be a freezable facet");
224:                } catch {
225:                    // In case the transaction is reverted, we would imply that the target contract 
226:                    // would not be the DiamondCut facet. That's enough to verify the target facet.
227:                    // In other words, the DiamondCut facet must implement the isDiamondCutFacet function 
228:                    // and return the legitimate value only.
229:                }
230:
231:            } catch {
232:                // In case the transaction is reverted, we would imply that the target contract 
233:                // would not be the DiamondCut facet. That's enough to verify the target facet.
234:                // In other words, the DiamondCut facet must implement the isDiamondCutFacet function 
235:                // and return the legitimate value only.
236:            }
237:        }
238:
239:        ds.selectorToFacet[_selector] = SelectorToFacet({
240:            facetAddress: _facet,
241:            selectorPosition: selectorPosition,
242:            isFreezable: _isSelectorFreezable
243:        });
244:        ds.facetToSelectors[_facet].selectors.push(_selector);
245:    }

Code snippet 8 below provides the DiamondCutFacetVerificationHelper library required by the updated _addOneFunction function.

The DiamondCutFacetVerificationHelper.functionStaticCall() function (L8 - 10) would be adopted by the _addOneFunction function in L219 - 220 in the code snippet 7 above.

Meanwhile, the DiamondCutFacetVerificationHelper.decodeBool() function (L12 - 14) would be adopted by the _addOneFunction function in L222 in the above code snippet 7.

SNIPPET: 8
FILE: (NEW FILE) https://github.com/code-423n4/2022-10-zksync/blob/main/ethereum/contracts/common/DiamondCutFacetVerificationHelper.sol

3:      pragma solidity ^0.8.0;
4:  
5:      import "@openzeppelin/contracts-upgradeable/utils/AddressUpgradeable.sol";
6:  
7:      library DiamondCutFacetVerificationHelper {
8:          function functionStaticCall(address target, bytes memory data) external view returns (bytes memory) {
9:              return AddressUpgradeable.functionStaticCall(target, data);
10:         }
11: 
12:         function decodeBool(bytes memory _input) external pure returns (bool result) {
13:             (result) = abi.decode(_input, (bool));
14:         }
15:     }

c4-judge commented 1 year ago

GalloDaSballo marked the issue as primary issue

miladpiri commented 1 year ago

We clearly stated that freezability is a very dangerous place, but we understand that. It sound pretty much the same as "accidentally transfer ownership to different address".

From the doc:

Note, that it is a very dangerous thing since the diamond proxy can freeze the upgrade system and then the diamond will be frozen forever.

However, I agree that mitigation step is interesting, probably can be a QA issue!

c4-sponsor commented 1 year ago

miladpiri marked the issue as disagree with severity

c4-sponsor commented 1 year ago

miladpiri marked the issue as sponsor acknowledged

GalloDaSballo commented 1 year ago

Per the sponsor comment, because the finding is out of scope I can only award the Refactoring suggestion, with QA Severity

R