MintingFactoryV2, BaseUpgradableMarketplace & KODAV3UpgradableGatedMarketplace – CoinFabrik Weblog

Introduction

CoinFabrik was requested to audit the contracts for the KnownOrigin venture. First we
will present a abstract of our discoveries after which we’ll present the main points of our
findings.

Scope

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

The audited information are:

● /contracts/market/KODAV3UpgradableGatedMarketplace.sol:
Accommodates the contract used to deploy gated marketplaces.
● /contracts/market/BaseUpgradableMarketplace.sol: Accommodates
the BaseUpgradableMarketPlace contract. This contract can be utilized to
make upgradable marketplaces, together with a number of utilities that simplify its
creation.
● /contracts/minter/MintingFactoryV2.sol: Accommodates the
MintingFactoryV2 contract. This contract is an upgradeable glue contract
that binds a number of contracts to facilitate the minting course of. It’s supposed to
change the MintingFactory contract whereas permitting for gated marketplaces.
● /contracts/entry/IKOAccessControlsLookup.sol: It accommodates the
interface definition for the contract liable for dealing with authorization of
completely different roles within the system.
● /contracts/core/IKODAV3.sol: It accommodates the interface definition for the
KODAV3 token.
● /contracts/market/IKODAV3Marketplace.sol: Accommodates interface
definitions for various sorts of market contracts.
● /contracts/core/IKODAV3Minter.sol: Accommodates the interface definition for
the KODA Minter contract (model 3).
● /contracts/collab/ICollabRoyaltiesRegistry.sol: Accommodates the
interface definition for the royalties registry.

The scope of the audit is proscribed to these information. No different information on this repository had been
audited. Its dependencies are assumed to work based on their documentation.
Additionally, no checks had been reviewed for this audit.
Fixes had been checked on commit 3bcd94f66e5d0f6b38881fd52971c13dd08b6974.

After that the event group fastened a bug concerning royalties distribution. We
checked that repair on commit 9cd490474667bf021c7b0c1e8b3c697fc3072f3a and no
new safety points had been discovered within the audited information.

Analyses

With out being restricted to them, the audit course of included the next analyses:

● Arithmetic errors
● Outdated model of Solidity compiler
● Race situations
● Reentrancy assaults
● Misuse of block timestamps
● Denial of service assaults
● Extreme fuel utilization
● Lacking or misused operate qualifiers
● Needlessly complicated code and contract interactions
● Poor or nonexistent error dealing with
● Inadequate validation of the enter parameters
● Incorrect dealing with of cryptographic signatures
● Centralization and upgradeability

Abstract of Findings

We discovered no important or medium points. A number of minor points had been discovered. Additionally,
a number of enhancements had been proposed.
All safety points had been both resolved, mitigated or acknowledged by the
improvement group. Some enhancements had been carried out by the event
group.

Safety Points

Privileged Roles

These are the privileged roles that we recognized on every of the audited contracts.

MintingFactoryV2

Admin

An deal with with the admin position can:

● Improve the contract.
● Set and unset the frequency override for an deal with.
● Set the minting interval.
● Set the royalties registry.
● Set the utmost mints in a interval.

The accessControls contract controls if an deal with has the admin position through the
hasAdminRole() operate.

Verified Artist

An deal with with the verified artist position can mint batches for itself utilizing the
following capabilities:

● mintBatchEdition()
● mintBatchEditionGatedOnly()
● mintBatchEditionGatedAndPublic()
● mintBatchEditionOnly()

Minting is ruled by the restrictions set by the admin.
The accessControls contract controls if an deal with has the verified artist position through
the isVerifiedArtist() operate. The required cryptographic proof is forwarded
to this operate.

Verified Artist Proxy

An deal with with the verified artist proxy position can mint batches on behalf of a
creator it represents utilizing the next capabilities:

● mintBatchEditionAsProxy()
● mintBatchEditionGatedOnlyAsProxy()
● mintBatchEditionGatedAndPublicAsProxy()
● mintBatchEditionOnlyAsProxy()

Minting is ruled by the restrictions set by the admin.
The accessControls contract controls if an deal with is a verified artist proxy for a
creator through the isVerifiedArtistProxy() operate.

BaseUpgradableMarketplace

Admin

An deal with with the admin position can:

● Improve the contract.
● Change the accessControls contract used within the contract through the
updateAccessControls() operate. So as to take action, it must have the
admin position in each the previous and new accessControls contracts.
● Switch away ERC20 tokens owned by the contract through the recoverERC20()
operate.
● Switch ether away owned by the contract the recoverStuckETH()
operate.
● Replace the platform commision through the
updatePlatformPrimaryComission() operate. This worth will not be utilized in
this contract however could also be utilized in a derived contract.
● Replace the modulo worth through the updateModulo() operate. This worth is barely
used within the non-public and non-invoked _handleSaleFunds() operate and will
even be utilized in a derived contract.
● Replace the minBidAmount through the updateMinBidAmount() operate. This
worth will not be used on this contract however could also be utilized in a derived contract.
● Replace the bidLockupPeriod through the updateBidLockupPeriod() operate.
This worth is barely used within the non-public and non-invoked _getLockupTime()
operate and may be utilized in a derived contract.

● Replace the platformAccount worth through the updatePlatformAccount()
operate. This worth is barely used within the non-public and non-invoked
_handleSaleFunds() operate and may be utilized in a derived contract.
● Pause and resume the contract through the pause() and unpause() capabilities.

The accessControls contract controls if an deal with has the admin position through the
hasAdminRole() operate.
Different capabilities in derived contracts could also be allowed to an admin solely through the
onlyAdmin() modifier and/or to the admin and different roles through the
onlyCreatorContractOrAdmin() modifier.

Contract and Creator

Whereas there are not any capabilities marked with the onlyCreatorContractOrAdmin()
modifier on this contract, this modifier can be utilized to limit the execution of
capabilities to both an admin, a contract or the issuer of the version handed as a
parameter. Checking that an deal with has a contract or admin position is made through the
accessControls.hasContractOrAdminRole() operate. To test if an deal with
corresponds to an editionId the koda.getCreatorOfEdition() operate is used.

KODAV3UpgradableGatedMarketplace

This contract inherits all of the roles and capabilities outlined within the
BaseUpgradableMarketplace contract. No new roles are added, however the unique
roles get extra capabilities.

Admin

Moreover the capabilities outlined in BaseUpgradableMarketplace contract, an
deal with with the admin position can:

● Change the fundsReceiver of a sale through the updateFundsReceiver()
operate.
● Change the maxEditionId of a sale through the updateMaxEditionId()
operate.
● Replace the creator of a sale through the updateCreator() operate.
● Override the fee for a sale through the
setKoCommissionOverrideForSale() operate.

Admin and Contract

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

● Create gross sales for any version through the createSale() and
createSaleWithPhases() capabilities.
● Create and take away phases of gross sales for any version through the createPhase(),
createPhases() and removePhase() capabilities.
● Pause or resume any sale through the toggleSalePause() operate.

Creator

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

● Create gross sales for any version whose editionId corresponds to them through the
createSale() and createSaleWithPhases() capabilities.
● Create and take away phases of gross sales for any version whose editionId
corresponds to them through the createPhase(), createPhases() and
removePhase() capabilities.
● Pause or resume any sale whose editionId corresponds to them through the
toggleSalePause() operate.

Safety Points Discovered

Severity Classification

Safety dangers are categorised as follows:

● Essential: These are points that we handle to take advantage of. They compromise the
system critically. They have to be fastened instantly.
● Medium: These are probably exploitable points. Although we didn’t
handle to take advantage of them or their impression will not be clear, they could symbolize a
safety threat within the close to future. We recommend fixing them as quickly as attainable.
● Minor: These points symbolize issues which are comparatively small or troublesome
to reap the benefits of however might be exploited together with different points.
These sorts of points don’t block deployments in manufacturing environments.
They need to be taken under consideration and be fastened when attainable.

Points Standing

A problem detected by this audit can have 4 distinct statuses:

● Unresolved: The problem has not been resolved.
● Acknowledged: The problem stays within the code however is a results of an intentional
choice.

● Resolved: Adjusted program implementation to remove the chance.
● Partially resolved: Adjusted program implementation to remove a part of the
threat. The opposite half stays within the code however is a results of an intentional
choice.
● Mitigated: Carried out actions to attenuate the impression or chance of the
threat.

Essential Severity Points

No points discovered.

Medium Severity Points

No points discovered.

Minor Severity Points

MI-01 Attainable Reentrancy Points

Location:

● contracts/minter/MintingFactoryV2.sol

Whereas accessControls, koda, market, gatedMarketplace and
royaltiesRegistry contracts are thought-about reliable, a artful attacker might
finally discover a manner to make use of these to set off a reentrancy assault within the
MintingFactoryV2 contract.

Advice

Mark all of the MintingFactoryV2.mint*() capabilities as non-reentrant utilizing the
nonReentrant modifier outlined within the OpenZeppelin library. Reentrancy points are
not straightforward to identify and the checks-effects-interaction sample can’t be trivially
utilized on this contract.

Standing

Acknowledged.

MI-02 Outdated Solidity Model

The solidity model declared within the audited contracts is 0.8.4. The most recent model
launched on the time of this audit is 0.8.13.

Advice

Take a look at all contracts with the most recent model and alter the pragma solidity
statements.

Standing

Acknowledged.

MI-03 Unchecked Switch

Location:

● contracts/market/BaseUpgradableMarketplace.sol:92

When the admin recovers ERC20 tokens within the BaseUpgradableMarketplace
contract, the contract doesn’t management if the switch to the brand new deal with is made or
not.
This concern is minor as a result of:

● Solely the admin can get better ERC20 tokens.
● If the test fails, the one distinction is {that a} AdminRecoverERC20 occasion is
emitted when it shouldn’t be.

Advice

Use OpenZeppelin’s safeTransfer() operate to switch ERC20s.

Standing

Resolved. Checked on commit 3bcd94f66e5d0f6b38881fd52971c13dd08b6974.

MI-04 Attainable DOS on KODAV3UpgradableGatedMarketplace.mint()

Location:

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

When KODAV3UpgradableGatedMarketplace.mint() is named, finally
BaseUpgradableMarketplace._handleSalesFunds() is named a number of occasions. In
this operate, 2 ether transfers are made, one to the platformAddress and the
different to the fundsReceiver of the sale.
If any of these addresses rejects the switch, then the minting is aborted.
The severity of this concern was lowered as a result of neither the platform nor the fund
receiver has clear financial incentives to do that and the DOS is transient.

Advice

Use the withdrawal sample to switch funds to these addresses. This will additionally
lead to fuel financial savings for a number of mints, as a single switch can be required for
every deal with.

Standing

Mitigated. The event group knowledgeable us that they don’t wish to introduce the
pull sample at this level as a result of UX complexity. mitigation might be achieved by an
admin operating the
KODAV3UpgradableGatedMarketplace.updateFundsReceiver() operate. The
improvement group additionally informs that the funds handler is derived from the collab
registry, to allow them to additionally disable minting entry to the malicious artists if discovered and
additionally replace the funds handler on this situation.

Enhancements

This stuff don’t symbolize a safety threat. They’re greatest practices that we
counsel implementing.

Desk

Particulars

EN-01 Time Lock Mechanism to Switch Funds and Improve

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

If an attacker takes management of an deal with with the admin position, it might steal all of the
funds belonging to a BaseUpgradableMarketplace derived contract, together with

https://docs.soliditylang.org/en/v0.8.13/common-patterns.html#withdrawal-from-contracts

KODAV3UpgradableGatedMarketplace. This operation could also be made immediately,
giving the actual admins no recourse to cease the operation.

Advice

Make the operations to get better ERC20s and ether, and likewise the improve of the
contract, time locked. This is able to give the likelihood to stop this assault from
taking place.

Standing

Not carried out. The event group acknowledges this suggestion however
won’t implement it.

EN-02 Lacking Non-Zero Checks

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

The BaseUpgradableMarketplace contract is lacking the checks for zero
addresses, that are carried out in its little one contract
KODAV3UpgradableGatedMarketplace.

This will result in utilizing an uninitialized deal with as goal for a switch, dropping these
funds.

Advice

Add the non-zero checks to the next operate parameters:

● _platformAccount within the BaseUpgradableMarketplace.initialize()
operate.
● _recipient within the BaseUpgradableMarketplace.recoverStuckETH()
operate.
● _newPlatformAccount within the
BaseUpgradableMarketplace.updatePlatformAccount() operate.

Standing

Carried out. Checked on commit 3bcd94f66e5d0f6b38881fd52971c13dd08b6974.

EN-03 Commissions Calculations

Location:

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

There are a number of issues that may be improved within the commissions calculations:

1. Decouple modulo for every commision. Now if the admin modifications the modulo
   then all of the commissions can be altered. It might be higher for every
   fee to have its personal modulo.
2. The worth for koCommissionOverrideForSale[_saleId].koCommission
   (set in KODAV3UpgradableGatedMarketplace.sol) and
   platformPrimaryCommission (set in BaseUpgradableMarketplace.sol)
   must be lower than its corresponding modulo.
3. Provided that the present provide of wei is about 1.2e26 , the multiplication in
   2 line 147 of BaseUpgradableMarketplace.sol wouldn’t overflow if written
   as msg.worth * _platformCommission / modulo, on condition that _platform fee is lower than 10 50. Checking that’s lower than 10 40 would be sure that it might not overflow for millennia. And by multiplying earlier than dividing the calculation of the fee can be extra exact. If different blockchains are added (completely different from ethereum), these numbers must be revised
   accordingly to keep away from overflows.

Standing

Partially carried out. Hereunder there may be the break up of this suggestion
implementation:

1. Not carried out.
2. Not carried out.
3. Carried out. Checked on commit
   3bcd94f66e5d0f6b38881fd52971c13dd08b6974.

Different Concerns

The issues said on this part will not be proper or incorrect. We don’t counsel
any motion to repair them. However we think about that they could be of curiosity for different
stakeholders of the venture, together with customers of the audited contracts, homeowners or
venture buyers.

Trusted Contracts

When analyzing the MintingFactoryV2 contract we assumed that the
accessControls, koda, market, gatedMarketplace and royaltiesRegistry contracts are reliable. These contracts are set within the initialization section, which is invoked through the deployment of the contract.
When analyzing the KODAV3UpgradableGatedMarketplace contract and its guardian,
BaseUpgradableMarketplace, we assumed that the accessControls and koda
contracts are reliable. These contracts are set within the initialization section, which
is invoked through the deployment of the contract. The accessControls contract can
even be modified by an admin, utilizing the
BaseUpgradableMarketplace.updateAccessControls() operate.

Centralization and Upgrades

The contracts MintingFactoryV2, KODAV3UpgradableGatedMarketplace and
BaseUpgradableMarketplace are fairly centralized. Particularly, an deal with with
the admin position might set off an improve, so with the ability to run any code. An
administrator may switch any ERC20 or ether belonging to a
BaseUpgradableMarketplace derived contract, together with
KODAV3UpgradableGatedMarketplace, to any arbitrary deal with.
Within the KODAV3UpgradableGatedMarketplace contract, addresses with the contract
position even have vital capabilities to intrude with any sale.

Changelog

● 2022-04-12 – Preliminary report primarily based on commit
d592c5f4fa4e0b6fc65a1fce43e302706aedf607.
● 2022-04-18 – Verify fixes on commit
3bcd94f66e5d0f6b38881fd52971c13dd08b6974.
● 2022-04-28 – Verify bug repair on commit
9cd490474667bf021c7b0c1e8b3c697fc3072f3a.

Disclaimer: This audit report will not be a safety guarantee, funding recommendation, or an
approval of the KnownOrigin venture since CoinFabrik has not reviewed its
platform. Furthermore, it doesn’t present a sensible contract code faultlessness
assure.