Zora Coinbase Creator Coin

Description

In the ZoraV4CoinHook contract, the migrateLiquidity function facilitates the migration of liquidity from an old hook to a new one. However, the function does not remove the old pool key from the poolCoins mapping after migration. This omission can result in stale state, where the old pool key remains associated with its coin and positions, potentially allowing swaps or other operations to be executed on liquidity that has already been migrated. This can lead to unexpected behavior, inconsistencies in reward distribution, and possible security risks if the migrated liquidity is assumed to be unavailable but is still referenced in the contract's state.

Recommendation

It is advisable to remove the old pool key from the poolCoins mapping after a successful migration. This ensures that the contract state accurately reflects the current liquidity configuration and prevents further interactions with migrated positions. The removal can be performed by deleting the mapping entry for the old pool key within the migrateLiquidity function after migration is complete. Additionally, review any related logic to ensure that stale references do not persist elsewhere in the contract.

Zora

Fixed in Commit #32d683d.

Cantina

Verified.

Description

ZoraV4CoinHook.migrateLiquidity(...) allows a coin owner to migrate liquidity positions from an old hook to a new one, which may be desirable if the new hook adds new functionality or fixes any issues in the old hook.

While migrateLiquidity(...) checks if an upgrade path has been registered by the protocol from the old hook to the new hook via HookUpgradeGate.isRegisteredUpgradePath(oldHook, newHook), it does not check if the newHook is registered in the hook registry via ZoraHookRegistry.isRegisteredHook(newHook). This allows migration to a new hook that is unregistered but has an upgrade path available, which may happen if the protocol unregisters a hook via removeHooks(...) in ZoraHookRegistry but misses to remove its upgrade path in HookUpgradeGate.

Additionally, HookUpgradeGate.registerUpgradePath(...) allows protocol to register optional upgrade paths for hooks. However, it does not check if the upgrade path is being registered for registered hooks via ZoraHookRegistry.isRegisteredHook(hook). This allows accidentally registering upgrade paths to unregistered hooks, which will allow coin owners to migrate their liquidity to unregistered hooks because migration also does not check for unregistered hooks currently.

Recommendation

Consider:

1. Checking ZoraHookRegistry.isRegisteredHook(newHook) during migration along with HookUpgradeGate.isRegisteredUpgradePath(oldHook, newHook).
2. Ensuring that hooks are simultaneously unregistered and their upgrade paths removed.
3. Checking isRegisteredHook(hook) in HookUpgradeGate.registerUpgradePath(...).

Zora

The hook registration is more informational for 3rd parties and does not enforce any security on the actual implementation. Acknowledged.

Cantina

Acknowledged.

Description

Creator coins are minted with a total supply of 10e9 tokens with half of that (500 * 10e6) allocated to the liquidity pool. The remaining half is claimable by the creator over a vesting period that is expected to be 5 years. However, this constant is set to CREATOR_VESTING_DURATION = 5 * 365 days; // 5 years, which ignores leap years. Given that there is 1 leap year every four years, this creator vesting duration is 1.25 days less than expected.

Recommendation

Consider using the more accurate calculation of 5 * 365.25 days for CREATOR_VESTING_DURATION to account for leap years.

Description

Swaps with native ETH currency will fail if one of the reward recipients cannot receive native ETH because CoinRewardsV4.distributeMarketRewards(...) in ZoraV4CoinHook._afterSwap() will revert in _transferCurrency().

Finding Description

ZoraV4CoinHook._afterSwap() calls CoinRewardsV4.distributeMarketRewards(...) to distribute rewards to all stakeholders. However, if the swap currency is native ETH and any of the five stakeholders of payoutRecipient, platformReferrer, protocolRewardRecipient, doppler or tradeReferrer is accidentally configured to not receive native ETH, then _transferCurrency() will revert with ICoin.EthTransferFailed. All coin swaps with native ETH currency will fail.

However, it is very unlikely that any of the five stakeholders will reject native ETH. Besides griefing, there is no incentive for external stakeholders of platformReferrer, doppler or tradeReferrer to do so.

Note: This was reported and acknowledged in the previous review https://cantina.xyz/code/e4fa76c6-c969-462c-9676-e7d4d60fc1dd/findings/7

Recommendation

Consider:

1. Emitting an event instead of reverting for failed ETH reward transfers.
2. Remitting the rewards in WETH instead of native ETH.
3. Adding a try/catch block to skip over reverts.

Zora

If the platform referrer is immutable; if it is set to an address that cannot receive ETH, it can brick swaps on the coin, as they would revert. Both the creator recipient and trade referral are changeable, the former via updating the payout recipient on the coin, and the latter via updating the trade referrer argument when doing a swap; therefore we do not need to worry about permanently bricking swaps, and don't need a backup recipient.

Commit #9c6d241

Cantina

Fixed in Commit #9c6d241 by introducing a backupRecipient parameter for _transferCurrency(...), which is set to protocolRewardRecipient and sent the ETH rewards if transfer fails for the initially set platformReferrer and doppler recipients.

Description

The codebase currently defines coin supply and allocation constants in multiple libraries, such as CoinConstants, MarketConstants, and CreatorCoinConstants. For example, CoinConstants.POOL_LAUNCH_SUPPLY and MarketConstants.CONTENT_COIN_MARKET_SUPPLY represent the same value but are declared separately. This duplication introduces a risk of inconsistency if values are updated in one location but not the other, potentially resulting in mismatches in total supply, market supply, or creator allocations. Additionally, creator and content coin-related constants are scattered across these files, making the codebase harder to maintain and increasing the likelihood of errors during upgrades or refactoring. The lack of dynamic relationships between these constants further exacerbates the risk, as dependent values are not automatically updated when base values change.

Recommendation

To mitigate these risks, it is recommended to centralize all coin-related constants into dedicated libraries, such as CreatorCoinConstants for creator coins and a separate library for content coins. Move all relevant values from CoinConstants and MarketConstants to ensure a single source of truth for each coin type. Where possible, define dependent values dynamically (e.g., CREATOR_VESTING_SUPPLY = TOTAL_SUPPLY - CREATOR_COIN_MARKET_SUPPLY) to maintain consistency and reduce manual errors. This restructuring will improve maintainability, simplify future upgrades, and ensure that supply and allocation logic remains coherent throughout the protocol.

Zora

All constants were consolidated in Commit #b571fe5.

Cantina

Verified. Although, the dynamic constant values risk was not addressed and is still present.

Description

The receive() function in the ZoraV4CoinHook contract is restricted by the onlyPoolManager modifier, preventing direct ETH transfers from any address except the PoolManager. While this restriction reduces the risk of accidental or malicious ETH deposits, it does not fully prevent ETH from being sent to the contract through other mechanisms, such as self-destruct operations from other contracts or protocol-level rewards. As a result, ETH could become stuck in the contract with no mechanism to recover or withdraw these funds, leading to potential loss of assets.

Recommendation

Consider implementing a recovery mechanism that allows the contract owner or a designated administrator to withdraw any ETH that may become stuck in the contract. This could be achieved by adding a withdrawal function with appropriate access controls. Alternatively, evaluate whether the receive function is necessary at all, and if not, remove it to further reduce the attack surface. If the contract must accept ETH for protocol reasons, ensure there is a clear and secure process for handling and recovering these funds.

Zora

We don't want to include ownership ability on the coin hook even for ETH recovery and if someone does a self-destruct ideally they know they are locking those funds. There shouldn't be any risk in adding the receive function here since it is gated and not having a receive function on the bytecode level is essentially solidity generating an revert on any received ETH rather than a entirely different type of contract on the bytecode level.

Cantina

Acknowledged.

Description

The ZoraHookRegistry contract is upgradeable and uses an initializer function, but it does not call _disableInitializers() in its constructor. This omission leaves the contract vulnerable to potential re-initialization attacks, where an attacker could call the initializer on the implementation contract and manipulate critical state variables or ownership. This is a well-known risk for upgradeable contracts that do not explicitly disable initializers on the implementation. However, the client has reported that this contract is already deployed and actively used, making direct remediation challenging.

Recommendation

Ideally, _disableInitializers() should be called in the constructor of the implementation contract to prevent any future initialization attempts on the implementation itself. For contracts already deployed, consider deploying a patched version and migrating usage to the new contract if feasible. Alternatively, closely monitor the implementation address for any suspicious activity and ensure that only proxy contracts interact with the registry. If migration is not possible, document the risk and educate protocol users and maintainers about the potential vulnerability.

Zora

This contract is not upgradeable, thus the initialize function is meant only to be called once. Adding _disableInitializers() to the constructor breaks this flow and isn't possible. A comment to describe the deployment constraints / flow may be helpful here to clarify.

Comment added in Commit #190a515

Cantina

Verified.

Description

The documentation for the ContentCoin contract states that content coins use creator coins as their backing currency. However, this is not enforced in the contract logic, and the client has clarified that content coins can use any currency as their backing, including ETH or other tokens. This discrepancy between the documentation and actual implementation can lead to confusion for integrators, auditors, and users, potentially resulting in incorrect assumptions about protocol behavior or integration requirements.

Recommendation

Update the documentation and comments in the ContentCoin contract to accurately reflect the intended functionality, specifying that content coins can use any currency as their backing. Ensure that all references to creator coins as the exclusive backing currency are removed or clarified. Additionally, review related documentation and developer guides to maintain consistency across the codebase and prevent future misunderstandings.

Zora

Fixed in Commit #c0da39f.

Cantina

Verified.

Description

ContractVersionBase is inherited by BaseCoin, ZoraV4CoinHook and ZoraFactoryImpl, all of which will return 2.2.0 for contractVersion(). Even if only the coin is upgraded to a new version at some point in future, all of them will return the same new upgraded version. Unless they are treated as a unified version for tracking reasons, using the same version across upgraded and not upgraded contracts is misleading.

Recommendation

Consider different versioning for different contracts.

Description

BaseCoin inherits ReentrancyGuardUpgradeable, but this is not required given that nonReentrant is never used on any of its functions. This appears to be supporting legacy functionality that has since been removed.

Recommendation

Consider removing ReentrancyGuardUpgradeable and any other unneeded inheritance to avoid bloat, improve readability and prevent any unexpected behavior in future.

Description

BaseCoin uses the initializer modifier on constructor, where the intent is to prevent calls to initialize on the implementation itself when used in the proxy pattern. While this appears to effectively perform the same functionality as calling _disableInitializers() within the constructor, this is not a widely-used pattern.

Recommendation

Consider using _disableInitializers() within the constructor to lock implementation coin contracts designed to be cloned.

Description

There are some stale/incorrect and missing comments across the codebase, which reduce readability. Some examples are:

1. * @notice Content coin implementation that uses creator coins as backing currency notes that content coins are only backed by their creator coin, but this is not enforced because they may be backed by ETH or even other content coins.
2. claimVesting() has /// @dev Optimized for frequent calls from Uniswap V4 hooks, but this is never called from the hook.
3. distributeMarketRewards() is missing @param for coinType.
4. unlockCallback() has /// @notice Internal fn called when the PoolManager is unlocked. Used to mint initial liquidity positions., but is missing that this is also used to burn all positions during migration.
5. initializeFromMigration(...) has // Verify that the caller (new hook) is authorized to perform this migration, but the caller is the old hook.

Recommendation

Consider adding/fixing the above comments.

Description

convertDeltaToPositiveUint128(...) downcasts int256 delta to uint128(uint256(delta)) as follows:

function convertDeltaToPositiveUint128(int256 delta) internal pure returns (uint128) {    if (delta < 0) {        revert SafeCast.SafeCastOverflow();    }    return uint128(uint256(delta));}

While delta values higher than uint128 may be unlikely, forced downcasting of delta is nevertheless risky for any edge-case scenarios.

Recommendation

Consider using an appropriate safe cast function or adding a bounds check delta <= type(uint128).max as best practice.

Description

Content coins support postDeployHook, which is used to implement afterCoinDeploy(...) for supporting initial token purchases immediately at deployment. However, Creator coin does not support this functionality, which may be desirable.

Recommendation

Consider unifying the Creator coin functionality with that of Content coin by supporting postDeployHook functionality.