Minting Factory V2 – Base Upgradable Marketplace & KODAV3 Upgradable Gated Marketplace

Introduction

CoinFabrik was asked to audit the contracts for the KnownOrigin project. First we will provide a summary of our discoveries and then we will show the details of our findings.

Scope

The contracts audited are from the https://github.com/knownorigin/known-origin-contracts-v3.git git repository. The audit is based on the commit d592c5f4fa4e0b6fc65a1fce43e302706aedf607.

The audited files are:

• /contracts/marketplace/KODAV3UpgradableGatedMarketplace.sol:Contains the contract used to deploy gated marketplaces.
• /contracts/marketplace/BaseUpgradableMarketplace.sol: Contains the BaseUpgradableMarketPlace contract. This contract can be used to make upgradable marketplaces, including several utilities that simplify its creation.
• /contracts/minter/MintingFactoryV2.sol: Contains the MintingFactoryV2 contract. This contract is an upgradeable glue contract that binds several contracts to facilitate the minting process. It is intended to replace the MintingFactory contract while allowing for gated marketplaces.
• /contracts/access/IKOAccessControlsLookup.sol: It contains the interface definition for the contract responsible for handling authorization of different roles in the system.
• /contracts/core/IKODAV3.sol: It contains the interface definition for the KODAV3 token.
• /contracts/marketplace/IKODAV3Marketplace.sol: Contains interface definitions for different kinds of marketplace contracts.
• /contracts/core/IKODAV3Minter.sol: Contains the interface definition for the KODA Minter contract (version 3).
• /contracts/collab/ICollabRoyaltiesRegistry.sol: Contains the interface definition for the royalties registry.

The scope of the audit is limited to those files. No other files in this repository were audited. Its dependencies are assumed to work according to their documentation. Also, no tests were reviewed for this audit.
Fixes were checked on commit 3bcd94f66e5d0f6b38881fd52971c13dd08b6974.

After that the development team fixed a bug regarding royalties distribution. We checked that fix on commit 9cd490474667bf021c7b0c1e8b3c697fc3072f3a and no new security issues were found in the audited files.

 

Analyses

Without being limited to them, the audit process included the following analyses:

● Arithmetic errors
● Outdated version of Solidity compiler
● Race conditions
● Reentrancy attacks
● Misuse of block timestamps
● Denial of service attacks
● Excessive gas usage
● Missing or misused function qualifiers
● Needlessly complex code and contract interactions
● Poor or nonexistent error handling
● Insufficient validation of the input parameters
● Incorrect handling of cryptographic signatures
● Centralization and upgradeability

 Summary of Findings

 

We found no critical or medium issues. Several minor issues were found. Also, several enhancements were proposed.
All security issues were either resolved, mitigated or acknowledged by the development team. Some enhancements were implemented by the development team.

Security Issues

 

 Privileged Roles

 

These are the privileged roles that we identified on each of the audited contracts.

 MintingFactoryV2

Admin

An address with the admin role can:

● Upgrade the contract.
● Set and unset the frequency override for an address.
● Set the minting period.
● Set the royalties registry.
● Set the maximum mints in a period.

The accessControls contract controls if an address has the admin role via the hasAdminRole() function.

Verified Artist

An address with the verified artist role can mint batches for itself using the following functions:

• mintBatchEdition()
• mintBatchEditionGatedOnly()

• mintBatchEditionGatedAndPublic()
• mintBatchEditionOnly()

Minting is governed by the restrictions set by the admin.
The accessControls contract controls if an address has the verified artist role via the isVerifiedArtist() function. The required cryptographic proof is forwarded to this function.

Verified Artist Proxy

An address with the verified artist proxy role can mint batches on behalf of a creator it represents using the following functions:

• mintBatchEditionAsProxy()
• mintBatchEditionGatedOnlyAsProxy()
• mintBatchEditionGatedAndPublicAsProxy()
• mintBatchEditionOnlyAsProxy()

Minting is governed by the restrictions set by the admin.
The accessControls contract controls if an address is a verified artist proxy for a creator via the isVerifiedArtistProxy() function.

BaseUpgradableMarketplace

Admin

An address with the admin role can:

• Upgrade the contract.
• Change the accessControls contract used in the contract via theupdateAccessControls() function. In order to do so, it needs to have the admin role in both the old and new accessControls contracts.
• Transfer away ERC20 tokens owned by the contract via the recoverERC20()function.
• Transfer ether away owned by the contract the recoverStuckETH()function.
• Update the platform commision via the updatePlatformPrimaryComission() function. This value is not used in this contract but may be used in a derived contract.
• Update the modulo value via the updateModulo() function. This value is only used in the private and non-invoked _handleSaleFunds() function and may also be used in a derived contract.
• Update the minBidAmount via the updateMinBidAmount() function. This value is not used in this contract but may be used in a derived contract.
• Update the bidLockupPeriod via the updateBidLockupPeriod() function. This value is only used in the private and non-invoked _getLockupTime()function and may also be used in a derived contract.
• Update the platformAccount value via the updatePlatformAccount()function. This value is only used in the private and non-invoked_handleSaleFunds() function and may also be used in a derived contract.
• Pause and resume the contract via the pause() and unpause() functions.

The accessControls contract controls if an address has the admin role via the hasAdminRole() function.
Other functions in derived contracts may be allowed to an admin only via theonlyAdmin() modifier and/or to the admin and other roles via theonlyCreatorContractOrAdmin() modifier.

Contract and Creator

While there are no functions marked with the onlyCreatorContractOrAdmin() modifier in this contract, this modifier can be used to restrict the execution of functions to either an admin, a contract or the issuer of the edition passed as a parameter. Checking that an address has a contract or admin role is made via the accessControls.hasContractOrAdminRole() function. To check if an address corresponds to an editionId the koda.getCreatorOfEdition()  function is used.

 

KODAV3UpgradableGatedMarketplace

This contract inherits all the roles and capabilities defined in the BaseUpgradableMarketplace contract. No new roles are added, but the original roles get additional capabilities.

Admin

Besides the capabilities defined in BaseUpgradableMarketplace contract, an address with the admin role can:

• Change the fundsReceiver of a sale via the updateFundsReceiver()function.
• Change the maxEditionId of a sale via the updateMaxEditionId()function.
• Update the creator of a sale via the updateCreator() function.
• Override the commission for a sale via the setKoCommissionOverrideForSale() function.

Admin and Contract

An address with the admin and/or contract roles can:

• Create sales for any edition via the createSale() and createSaleWithPhases() functions.
• Create and remove phases of sales for any edition via the createPhase(),createPhases() and removePhase() functions.
• Pause or resume any sale via the toggleSalePause() function.

Creator

An address with the admin and/or contract roles can:

• Create sales for any edition whose editionId corresponds to them via the createSale() and createSaleWithPhases() functions.
• Create and remove phases of sales for any edition whose editionId corresponds to them via the createPhase(), createPhases() and removePhase() functions.
• Pause or resume any sale whose editionId corresponds to them via the toggleSalePause() function.

 

Security Issues Found

 

Severity Classification

Security risks are classified as follows:

• Critical: These are issues that we manage to exploit. They compromise the system seriously. They must be fixed immediately.
• Medium: These are potentially exploitable issues. Even though we did not manage to exploit them or their impact is not clear, they might represent a security risk in the near future. We suggest fixing them as soon as possible.
• Minor: These issues represent problems that are relatively small or difficult to take advantage of but can be exploited in combination with other issues.
  These kinds of issues do not block deployments in production environments. They should be taken into account and be fixed when possible.

 

Issues Status

An issue detected by this audit can have four distinct statuses:

• Unresolved: The issue has not been resolved.
• Acknowledged: The issue remains in the code but is a result of an intentional decision.
• Resolved: Adjusted program implementation to eliminate the risk.
• Partially resolved: Adjusted program implementation to eliminate part of the risk. The other part remains in the code but is a result of an intentional decision.
• Mitigated: Implemented actions to minimize the impact or likelihood of the risk.

 

Critical Severity Issues

No issues found.

 

Medium Severity Issues

No issues found.

 

Minor Severity Issues

 

MI-01 Possible Reentrancy Issues

Location:

● contracts/minter/MintingFactoryV2.sol

While accessControls, koda, marketplace, gatedMarketplace and royaltiesRegistry contracts are considered trustworthy, a crafty attacker may eventually find a way to use those to trigger a reentrancy attack in the MintingFactoryV2 contract.

Recommendation

Mark all the MintingFactoryV2.mint*() functions as non-reentrant using the nonReentrant modifier defined in the OpenZeppelin library. Reentrancy issues are not easy to spot and the checks-effects-interaction pattern cannot be trivially applied in this contract.

Status

Acknowledged.

 

MI-02 Outdated Solidity Version

The solidity version declared in the audited contracts is 0.8.4. The latest version released at the time of this audit is 0.8.13.

Recommendation

Test all contracts with the latest version and change the pragma solidity statements.

Status

Acknowledged.

 

MI-03 Unchecked Transfer

Location:

● contracts/marketplace/BaseUpgradableMarketplace.sol:92

When the admin recovers ERC20 tokens in the BaseUpgradableMarketplace contract, the contract does not control if the transfer to the new address is made or not.
This issue is minor because:

• Only the admin can recover ERC20 tokens.
• If the check fails, the only difference is that a AdminRecoverERC20 event is emitted when it should not be.

Recommendation

Use OpenZeppelin’s safeTransfer() function to transfer ERC20s.

Status

Resolved. Checked on commit 3bcd94f66e5d0f6b38881fd52971c13dd08b6974.

 

MI-04 Possible DOS on KODAV3UpgradableGatedMarketplace.mint()

Location:

● contracts/marketplace/BaseUpgradableMarketplace.sol:149,152

When KODAV3UpgradableGatedMarketplace.mint() is called, eventually BaseUpgradableMarketplace._handleSalesFunds() is called several times. In this function, 2 ether transfers are made, one to the  platformAddress and the other to the fundsReceiver of the sale.
If any of those addresses rejects the transfer, then the minting is aborted. The severity of this issue was lowered because neither the platform nor the fund receiver has clear economic incentives to do this and the DOS is transient.

Recommendation

Use the withdrawal pattern to transfer funds to those addresses. This may also result in gas savings for multiple mints, as a single transfer would be required for each address.

Status

Mitigated. The development team informed us that they don’t want to introduce the pull pattern at this point due to UX complexity. mitigation can be achieved by an admin running the KODAV3UpgradableGatedMarketplace.updateFundsReceiver() function. The development team also informs that the funds handler is derived from the collab registry, so they can also disable minting access to the malicious  rtists if found and also update the funds handler in this scenario.

 

Enhancements

These items do not represent a security risk. They are best practices that we suggest implementing.

 

Table

 

 

Details

 

EN-01 Time Lock Mechanism to Transfer Funds and Upgrade

Location:
● contracts/marketplace/BaseUpgradableMarketplace.sol:87-100

If an attacker takes control of an address with the admin role, it can steal all the funds belonging to a BaseUpgradableMarketplace derived contract, including https://docs.soliditylang.org/en/v0.8.13/common-patterns.html#withdrawal-from-contracts KODAV3UpgradableGatedMarketplace. This operation may be made instantly, giving the real admins no recourse to stop the operation.

Recommendation

Make the operations to recover ERC20s and ether, and also the upgrade of the contract, time locked. This would give the possibility to prevent this attack from happening.

Status

Not implemented. The development team acknowledges this recommendation but will not implement it.

 

EN-02 Missing Non-Zero Checks

Location

● contracts/marketplace/BaseUpgradableMarketplace.sol:78,96,131

The BaseUpgradableMarketplace contract is missing the checks for zero addresses, which are implemented in its child contract KODAV3UpgradableGatedMarketplace.

This may lead to using an uninitialized address as target for a transfer, losing those funds.

Recommendation

Add the non-zero checks to the following function parameters:

● _platformAccount in the BaseUpgradableMarketplace.initialize()function.
● _recipient in the BaseUpgradableMarketplace.recoverStuckETH()function.
● _newPlatformAccount in the BaseUpgradableMarketplace.updatePlatformAccount() function.

Status

Implemented. Checked on commit 3bcd94f66e5d0f6b38881fd52971c13dd08b6974.

 

EN-03 Commissions Calculations

Location:

• contracts/marketplace/BaseUpgradableMarketplace.sol:29-32,58-67, 113-117, 146-154
• contracts/marketplace/KODAV3UpgradableGatedMarketplace.sol:70,102-105, 306-311, 396-401

There are several things that can be improved in the commissions calculations:

 

1. Decouple modulo for each commision. Now if the admin changes the module then all the commissions would be altered. It would be better for each commission to have its own modulo.
2. The value for koCommissionOverrideForSale[_saleId].koCommission (set in KODAV3UpgradableGatedMarketplace.sol) and platformPrimaryCommission (set in BaseUpgradableMarketplace.sol) should be less than its corresponding modulo.
3. Given that the current supply of wei is about 1.2e26 , the multiplication in 2 line 147 of BaseUpgradableMarketplace.sol would not overflow if written as msg.value * _platformCommission / modulo, given that _platform commission is less than 10 50. Checking that is less than 10 40 would ensure that it would not overflow for millennia. And by multiplying before dividing the calculation of the commission would be more precise. If other  lockchains are added (different from ethereum), these numbers should be revised accordingly to avoid overflows.

Status

Partially implemented. Hereunder there is the split of this recommendation
implementation:

 

1. Not implemented.
2. Not implemented.
3. Implemented. Checked on commit
   3bcd94f66e5d0f6b38881fd52971c13dd08b6974.

 

Other Considerations

The considerations stated in this section are not right or wrong. We do not suggest any action to fix them. But we consider that they may be of interest for other stakeholders of the project, including users of the audited contracts,  owners or project investors.

 

Trusted Contracts

When analyzing the MintingFactoryV2 contract we assumed that the accessControls, koda, marketplace, gatedMarketplace and royaltiesRegistry contracts are trustworthy. Those contracts are set in the initialization phase, which is invoked during the deployment of the contract.
When analyzing the KODAV3UpgradableGatedMarketplace contract and its parent, BaseUpgradableMarketplace, we assumed that the accessControls and koda contracts are trustworthy. Those contracts are set in the initialization phase, which is invoked during the deployment of the contract. The accessControls contract canalso be changed by an admin, using the BaseUpgradableMarketplace.updateAccessControls() function.

 

Centralization and Upgrades

The contracts MintingFactoryV2, KODAV3UpgradableGatedMarketplace and BaseUpgradableMarketplace are quite centralized. In particular, an address with the admin role may trigger an upgrade, so being able to run any code. An administrator may also transfer any ERC20 or ether belonging to a BaseUpgradableMarketplace derived contract, including KODAV3UpgradableGatedMarketplace, to any arbitrary address.
In the KODAV3UpgradableGatedMarketplace contract, addresses with the contract role also have significant capabilities to interfere with any sale.

 

Changelog

● 2022-04-12 – Initial report based on commit d592c5f4fa4e0b6fc65a1fce43e302706aedf607.
● 2022-04-18 – Check fixes on commit 3bcd94f66e5d0f6b38881fd52971c13dd08b6974.
● 2022-04-28 – Check bug fix on commit 9cd490474667bf021c7b0c1e8b3c697fc3072f3a.

Disclaimer: This audit report is not a security warranty, investment advice, or an approval of the KnownOrigin project since CoinFabrik has not reviewed its platform. Moreover, it does not  provide a smart contract code faultlessness guarantee.