c4-submissions commented 11 months ago

Lines of code

https://github.com/code-423n4/2023-10-zksync/blob/1fb4649b612fac7b4ee613df6f6b7d921ddd6b0d/code/contracts/ethereum/contracts/zksync/libraries/PriorityQueue.sol#L79 https://github.com/code-423n4/2023-10-zksync/blob/1fb4649b612fac7b4ee613df6f6b7d921ddd6b0d/code/contracts/ethereum/contracts/zksync/libraries/Diamond.sol#L252

Vulnerability details

Impact

I grouped this issue under one because the root causes are the same.

you can see when removing a function, in Diamond.sol, delete is called on a mapping in a struct. Link here
```
    // Finally, clean up the association with facet
    delete ds.selectorToFacet[_selector];
```
when trying to remove a priority operation from the queue in PriorityQueue.sol, delete is called on mapping in a struct Link here
```
function popFront(Queue storage _queue) internal returns (PriorityOperation memory priorityOperation) {
    require(!_queue.isEmpty(), "s"); // priority queue is empty

    // Save value into the stack to avoid double reading from the storage
    uint256 head = _queue.head;

    priorityOperation = _queue.data[head];
    delete _queue.data[head];
    _queue.head = head + 1;
}
```
When you delete a struct in Solidity, it will not delete the mapping within it. The delete keyword in Solidity sets every field in the struct to its default value. For integers, strings, arrays, and other simple data types, this means they will be set to zero, an empty string, or an empty array, respectively. However, for mappings, the delete keyword has no effect. This is because mappings are implemented as hash tables and the Ethereum Virtual Machine (EVM) does not keep track of which keys have been used in the mapping. As a result, it doesn't know how to "reset" a mapping. Therefore, when you delete a struct, the mapping within it will still retain its old data. Trying to delete such a structure from storage will likely result in data corruption, rendering the structure unusable. This can lead to potential security issues.

In Executor.sol when executing batches, it pops operations out of the priority queue. Link here

   /// @dev Pops the priority operations from the priority queue and returns a rolling hash of operations
    function _collectOperationsFromPriorityQueue(uint256 _nPriorityOps) internal returns (bytes32 concatHash) {
        concatHash = EMPTY_STRING_KECCAK;

        for (uint256 i = 0; i < _nPriorityOps; i = i.uncheckedInc()) {
            PriorityOperation memory priorityOp = s.priorityQueue.popFront();
            concatHash = keccak256(abi.encode(concatHash, priorityOp.canonicalTxHash));
        }
    }

are you can see under the hood it calls s.priorityQueue.popFront() where it pops the element at the head of the by calling delete on it, the issue here is as shown above, the mapping will still retain its data and clear other values not expected from the storage here it can lead to

priority operations are bricked.
Data corruption and storage collisions where unexpected values in the storage are erased, thereby bricking zkSync

In Diamond.sol when it is required to remove a function selector linked to a facet, Link here

   function _removeOneFunction(address _facet, bytes4 _selector) private {
      // ---some code----

        // Finally, clean up the association with facet
        delete ds.selectorToFacet[_selector];

        // If there are no selectors for facet then remove the facet from the list of known facets
        if (lastSelectorPosition == 0) {
            _removeFacet(_facet);
        }

it calls delete on the mapping of the function selector, here the function selector is still retained where there is Data corruption in Diamond.sol storage, it can render the zk L1 contract useless as access can be broken and certain functions will be inaccessible.

This will lead to the disruption of many functionalities in zkSync.io especially when using EIP-2535, The bug pathways(Undesired) will be so many.