<div align="center">
<center>
<img src="https://avatars.githubusercontent.com/u/167952721" height="350" alt="@offbeatsecurity" style="margin-bottom: 20px;">
<h1>ZKsync Capped Minter v2</h1>
<h3>December 20, 2024</h3>
<p>Prepared for ZKsync</p>
<p>Conducted by:</p>
<p>Kurt Willis (phaze)</p>
<p>Richie Humphrey (devtooligan)</p>
</center>
</div>

## About the **ZKsync Capped Minter v2**

ZKsync Era is a Layer 2 ZK rollup, a trustless protocol that uses cryptographic validity proofs to provide scalable and low-cost transactions on Ethereum.

The CappedMinter v2 is an updated version of a contract used for allocating ZK tokens by means of creating a contract which allows the owner to mint up to a preselected amount of tokens.

## About **Offbeat Security**

Offbeat Security is a boutique security company providing unique security solutions for complex and novel crypto projects. Our mission is to elevate the blockchain security landscape through invention and collaboration.

## Scope

The [l2-contracts/src/](https://github.com/ScopeLift/zk-governance/tree/4f2eabddf8c1952e7995bb9325c2ec59466e677b/l2-contracts/src) folder of the `zk-governance` repo was reviewed at commit [4f2eabd](https://github.com/ScopeLift/zk-governance/tree/4f2eabddf8c1952e7995bb9325c2ec59466e677b).

The following **2 contracts** were in scope:
- l2-contracts/src/ZkCappedMinterV2.sol
- l2-contracts/src/ZkCappedMinterV2Factory.sol

## Summary

The `ZkCappedMinterV2` contract has a limited attack surface as all functions are permissioned. There are several administrative functions which are minor modifications to OpenZeppelin library functions. The primary entry point, mint, is heavily permissioned with several preconditions enforced. There is no math or other areas of complexity, and there are no external integrations. The `ZkCappedMinterV2Factory` contract and the contracts it creates are effectively siloed from the minter due to the fact that the MINTER role must be granted before anything can be minted.

| Identifier | Title | Severity | Fixed |
| ---------- | ---------------------------- | ------------- | ----- |
| [L-01](#L-01-Bytecode-hash-mismatch-can-cause-incorrect-address-prediction) | Bytecode hash mismatch can cause incorrect address prediction | Low | Ack. Project will take care to deploy the factory with the correct bytecode hash. |
| [I-01](#I-01-Metadata-URI-can-be-modified-after-contract-closure) | Metadata URI can be modified after contract closure | Info | Ack. This is desired functionality. |

## Detailed Findings

## Low Findings

## [L-01] Bytecode hash mismatch can cause incorrect address prediction

### Summary

The `ZkCappedMinterV2Factory` contract accepts a bytecode hash as a constructor parameter without verifying it matches the actual deployment bytecode of `ZkCappedMinterV2`. This could lead to incorrect address predictions by the `getMinter()` function if the provided hash doesn't match the actual deployed contract bytecode.

### Description

The `ZkCappedMinterV2Factory` constructor accepts a `_bytecodeHash` parameter that is stored as an immutable variable:

constructor(bytes32 _bytecodeHash) {
  BYTECODE_HASH = _bytecodeHash;
}

This hash is used in `getMinter()` to predict deployment addresses:

addr = L2ContractHelper.computeCreate2Address(
  address(this), 
  salt, 
  BYTECODE_HASH,
  keccak256(abi.encode(_mintable, _admin, _cap, _startTime, _expirationTime))
);

Two potential issues arise:
1. The constructor parameter could be set incorrectly during deployment
2. Small changes in the contract's metadata (comments, whitespace, unused files) can change the bytecode hash, making it diverge from the provided value

If either occurs, `getMinter()` will return incorrect addresses, potentially causing issues for users and integrating protocols that rely on address prediction.

### Recommendation

Consider calculating the bytecode hash during construction instead of accepting it as a parameter. To calculate the bytecode hash, use the `hashL2Bytecode` function in the ZKsync [Utils](https://github.com/matter-labs/era-contracts/blob/main/system-contracts/contracts/libraries/Utils.sol#L82-L97) library.

This change ensures the bytecode hash always matches the actual deployment code and eliminates the possibility of configuration errors. It also makes the contract more maintainable as it automatically updates when the contract code changes.

## Informational Findings

## [I-01] Metadata URI can be modified after contract closure

The ZkCappedMinterV2 contract allows modification of the metadataURI even after the contract has been closed through the `close()` function. This contradicts the intent of the contract closure mechanism, which is designed to permanently disable all administrative operations.

The `close()` function is intended to permanently disable the contract's functionality, as indicated by its documentation:

/// @notice Permanently closes the contract, preventing any future minting.
/// @dev Once closed, the contract cannot be reopened and all minting operations will be permanently blocked.

However, while minting is properly blocked through the `_revertIfClosed()` check:

function mint(address _to, uint256 _amount) external {
    _revertIfClosed();
    // ... rest of function
}

The `setMetadataURI()` function remains callable by the admin even after closure:

function setMetadataURI(string memory _uri) external {
    _checkRole(DEFAULT_ADMIN_ROLE, msg.sender);
    metadataURI = _uri;
    emit MetadataURISet(_uri);
}

This creates an inconsistency in the contract's closed state behavior, where some admin functions are blocked while others remain active.

### Recommendation

Consider adding the `_revertIfClosed()` check to the `setMetadataURI()` function to maintain consistent behavior after contract closure:

function setMetadataURI(string memory _uri) external {
+   _revertIfClosed();
    _checkRole(DEFAULT_ADMIN_ROLE, msg.sender);
    metadataURI = _uri;
    emit MetadataURISet(_uri);
}

This change ensures that all administrative functions are consistently blocked once the contract is closed.