Nori A-1

Security Audit

December 13, 2022

Version 1.0.0

Presented by 0xMacro

Table of Contents

Introduction

This document includes the results of the security audit for Nori's smart contract code as found in the section titled ‘Source Code’. The security audit was performed by the Macro security team from October 17, 2022 to November 4, 2022.

The purpose of this audit is to review the source code of certain Nori Solidity contracts, and provide feedback on the design, architecture, and quality of the source code with an emphasis on validating the correctness and security of the software in its entirety.

Disclaimer: While Macro’s review is comprehensive and has surfaced some changes that should be made to the source code, this audit should not solely be relied upon for security, as no single audit is guaranteed to catch all possible bugs.

Overall Assessment

The following is an aggregation of issues found by the Macro Audit team:

Severity Count Acknowledged Won't Do Addressed
High 4 - - 4
Medium 6 - - 6
Low 4 - 1 3
Code Quality 9 - 1 8
Gas Optimization 4 - - 4

Nori was quick to respond to these issues.

Specification

Our understanding of the specification was based on the following sources:

Source Code

The following source code was reviewed during the audit:

Specifically, we audited the following contracts within this repository:

Contract SHA256
contracts/AccessPresetPausable.sol

7056fd9913a5c317b553d6a9fe4cc0c3322f242cc82fe50f77d231186f1e2396

contracts/ArrayLib.sol

d4b98ccb638f502664e3d23297d7c079c818691f093e9abb706d1c6ec9b03fea

contracts/BridgedPolygonNORI.sol

c443ac56c985b3f8f7fa8e236b7da7b159f9400b9e02944c8d3da9da8ff0a4d5

contracts/Certificate.sol

1d774367201dd40c4a50e7f9bc16f6830d5743c4727dafeadef15ccf439f088e

contracts/ERC20Preset.sol

36fa7705a131aaf5e36718a7274444b4c62fa19861b3125486fd5ae468789244

contracts/Errors.sol

7260b8c45c5fc7f32d3dfdd44b739cbd5f1e5911f6204dd927a904367c312bd2

contracts/LockedNORI.sol

7b052a885d0351eaa8d8a23da76f7828d2ec79f8f56496cbc624fb8b00048c45

contracts/LockedNORILib.sol

109fbd485c33632aa9bb5ce0aa5fb9d6bf6ffea39a50ebdd6cab34802fd9c28b

contracts/Market.sol

5cb548a2f4687d3b154536a5fb7f343502358e6f8d9ccf241745b870d2803f0e

contracts/NORI.sol

8b007002bce82582b9549341afee0f5c88b3a0f2546fa9327572786d8e6ae8ac

contracts/Removal.sol

2e8c5ca12ce5569fe4c5a6d4fb054ac187ca29d2bfc97d90be1a951e6ebf23b2

contracts/RemovalIdLib.sol

4a3b66289dcf6fd470c0efcd9c3a78ff71fde8cd71452e51dd19d1cacc8e47c4

contracts/RemovalsByYearLib.sol

f606b486d5306f3d72f791cda8b45cab1e9c879d9cb802d141564b79390913a8

contracts/RestrictedNORI.sol

8aa1b5d4cbaf616982c4ef019945d81225689f81f1cc63bd386257105c9dd185

contracts/RestrictedNORILib.sol

7cbdf2eea0fcfde36814fce6b46a2559aa2624ebe376f2b9a8add04541bde4df

Note: This document contains an audit solely of the Solidity contracts listed above. Specifically, the audit pertains only to the contracts themselves, and does not pertain to any other programs or scripts, including deployment scripts.

Issue Descriptions and Recommendations

Click on an issue to jump to it, or scroll down to see them all.

Security Level Reference

We quantify issues in three parts:

  1. The high/medium/low/spec-breaking impact of the issue:
    • How bad things can get (for a vulnerability)
    • The significance of an improvement (for a code quality issue)
    • The amount of gas saved (for a gas optimization)
  2. The high/medium/low likelihood of the issue:
    • How likely is the issue to occur (for a vulnerability)
  3. The overall critical/high/medium/low severity of the issue.

This third part – the severity level – is a summary of how much consideration the client should give to fixing the issue. We assign severity according to the table of guidelines below:

Severity Description
(C-x)
Critical

We recommend the client must fix the issue, no matter what, because not fixing would mean significant funds/assets WILL be lost.

(H-x)
High

We recommend the client must address the issue, no matter what, because not fixing would be very bad, or some funds/assets will be lost, or the code’s behavior is against the provided spec.

(M-x)
Medium

We recommend the client to seriously consider fixing the issue, as the implications of not fixing the issue are severe enough to impact the project significantly, albiet not in an existential manner.

(L-x)
Low

The risk is small, unlikely, or may not relevant to the project in a meaningful way.

Whether or not the project wants to develop a fix is up to the goals and needs of the project.

(Q-x)
Code Quality

The issue identified does not pose any obvious risk, but fixing could improve overall code quality, on-chain composability, developer ergonomics, or even certain aspects of protocol design.

(I-x)
Informational

Warnings and things to keep in mind when operating the protocol. No immediate action required.

(G-x)
Gas Optimizations

The presented optimization suggestion would save an amount of gas significant enough, in our opinion, to be worth the development cost of implementing it.

Issue Details

H-1

Calling Removal.release() does not fully un-list from the Market, allowing the burned removalId to be swapped into a new Certificate and the supplier to receive rewards

Topic
Spec
Status
Impact
Spec Breaking
Likelihood
Medium

Any listed amount of a Removal being released will be burned within Removal.sol, which prevents the subsequent call to Market.release() from properly un-listing the Removal.

See Removal.sol lines 692-694 below. Note that the _burn() is called prior to invoking _market.release():

function _releaseFromMarket(uint256 id, uint256 amount) internal {
  super._burn({from: this.marketAddress(), id: id, amount: amount});
  _market.release(id, amount);

Within Market.sol release() lines 277-286, the balanceOf() will be zero, preventing _removeActiveRemoval() from being invoked.

uint256 removalBalance = _removal.balanceOf({
  account: address(this),
  id: removalId
});
if (amount == removalBalance) {
  _removeActiveRemoval({
    removalId: removalId,
    supplierAddress: supplierAddress
  });
}

This leaves the removalId in the suppliers _listedSupply, such that a Market.sol swap() can potentially associate the orphaned removalId into a new Certificate and the supplier to receive unearned bpNori tokens, as long as the holdback percentage is not ≥ 100.

Remediations to Consider

Consider updating Market.sol release() logic to account for the tokens being burned within Removal.sol.

Response by Nori

Resolution: we elected to resolve this not by swapping the order of _burn and market.release, which creates a pattern that makes reentrancy vulnerabilities more possible with future changes (we get a Slither warning about that), but instead to change the check inside market.release to check whether the market contract’s balance for the removal in question is now 0, in which case the removal is removed from the sale queue.

H-2

Holders of RestrictedNORI tokens can claim bpNori tokens earlier than intended

Topic
Spec
Status
Impact
High
Likelihood
High

In RestrictedNORI.sol, holders should be able to claim their bpNori tokens based on a linear schedule mechanism, meaning that for a schedule duration of 10 years, a holder can claim 10% of his tokens after 1 year.

Due to how the claimable amount is calculated, a holder can claim more than 10% of tokens (even up to 100%) by repeatedly withdrawing some amount and transferring the remaining amount to another account.

Depending on how many holders are attached to the schedule, the consequences can differ.

Scenario 1: 1 holder is attached to the schedule

  1. A schedule is created with a 10 year schedule duration

  2. Holder_1 has 1000 tokens locked up

  3. After 1 year, Holder_1 claims his tokens by calling the withdrawFromSchedule function with the maximum claimable amount of 100 tokens (10% of 1000).

  4. Holder_1 transfers his remaining balance (900 tokens) to another account he owns, let’s call it Holder_1b.

  5. Holder_1b now can claim another 90 tokens (10% of 900).

As a consequence, the holder claimed 190 tokens instead of the allowed 100 tokens.

Scenario 2: Multiple holders are attached to the schedule

In case multiple holders are attached to the schedule, Holder_1 can repeat step 3-5 until the schedule’s maximum claimable amount is reached.

As a consequence, when enough balance is in the schedule pool, a holder can claim up to 100% of his tokens much earlier than intended by the schedule.

Remediations to Consider

When withdrawFromSchedule is called, the schedule.claimedAmountsByAddress is updated to reflect the total amount of claimed tokens for the specific holder’s address. Currently, this value is not being copied over to the new holder when tokens are transferred from one holder to another. Consider to update safeTransferFrom and safeBatchTransferFrom functions, so that the schedule.claimedAmountsByAddress amount is applied to the address where tokens are transferred to.

Response by Nori

Resolution: to minimize scope of the change we elected to restrict token transfers for this release with the intention of revisiting the transfer bookkeeping in a future release. safeTransferFrom and safeBatchTransferFrom are now disabled and the math that governs claimable tokens should not allow over-withdrawing for any given holder.

H-3

LockedNORI grantees can bypass vesting logic, withdrawing their full grant almost immediately

Topic
Spec
Status
Impact
High
Likelihood
High

LockedNORI.sol extends from deprecated/ERC777PresetPausablePermissioned.sol, which implements batchSend(). This method is not overridden to revert in LockedNORI.sol, allowing a grant recipient to transfer their unlocked amount to another address. This will draw down their balanceOf() amount but leave the vestedBalanceOf() amount unchanged, allowing the attacker to repeatedly siphon the vested balance amount via batchSend() until their full balance has been transferred. If sent to an address without a granting schedule, the full balance is immediately withdraw-able.

Remediations to Consider

Consider overriding batchSend() to revert, as send() presently does.

H-4

LockedNORI grantees can block calls to revokeUnreleasedTokens(), allowing them to withdraw their full amount upon vesting

Topic
Spec
Status
Impact
High
Likelihood
High

LockedNORI is a partial ERC777 implementation, which supports the _callTokensToSend() callback as part of _burn(). A malicious grantee can register an ERC1820-compliant contract as an ERC777TokenSender, which can be coded to revert on any send callbacks that the grantee did not initiate. This allows them to default-disallow LockedNORI sends, which will cause revokeUnreleasedTokens() to revert. They can subsequently allow sends prior to calling withdrawTo(), for any vested tokens.

Remediations to Consider

Consider overriding the ERC777’s _mint(), _burn(), and _send() functions with a version that does not call _callTokensToSend().

M-1

Any supplier that doesn’t accept ERC1155 tokens will revert when they are minted RestrictedNORI, breaking the entire market’s swap mechanism

Topic
Spec
Status
Impact
Medium
Likelihood
Medium

RestrictedNORI.sol implements ERC1155, which supports the onERC1155Received callback as part of the minting (see here), thus requiring recipients of token transfers (including burn and mint) to “accept” the reception of tokens.

Any supplier can register a contract’s address as the supplierAddress that does not accept ERC1155 tokens. As a result, the entire market’s swap mechanism would break, since it would revert whenever RestrictedNORIs are minted to the affected supplierAddress.

See Market.sol lines 812-815:

_restrictedNORI.mint({                      
  amount: restrictedSupplierFee,
  removalId: removalIds[i]
});

and the subsequent minting in RestrictedNORI.sol lines 448-451:

address supplierAddress = RemovalIdLib.supplierAddress({
  removalId: removalId
});

// audit: this is where the swap transaction reverts
super._mint({to: supplierAddress, id: projectId, amount: amount, data: ""});

Once the affected supplier has been selected as current supplier (thus _currentSupplierAddress points to the affected supplier), all subsequent calls to the market’s swap function will revert.

Note that the Market.sol’s swapFromSupplier function can still be used as long as the specified supplier address doesn’t reject on minting. However, one of the market’s core principle - of having a fair, consistent selling order - can only be achieved via the swap function.

Remediations to Consider

Consider adding appropriate error handling to Market.sol, so that the swap function doesn’t revert when minting is rejected by a supplier. Thus, whenever a supplier that doesn’t accept ERC1155 tokens has been identified, an appropriate event could be emitted signaling the releaser role to release the affected removals from the market.

M-2

BridgedPolygonNORI can be deposited beyond a recipient’s grant limit, permanently locking the bpNORI

Topic
Input Validation
Status
Impact
High
Likelihood
Low

The depositFor() function allows additional amounts to be deposited beyond a recipient’s grant amount. This locks the bpNORI amount in LockedNORI; balanceOf() will show the additional amount, but it cannot be withdrawn by the recipient.

Remediations to Consider

Consider disallowing deposits beyond recipient grant amount. The simplest solution may be to reduce grant creation+funding into a single transactional operation, such as implementing the recommendation of L-2 and also deleting the depositFor() function entirely. If this is not feasible, consider adding additional logic checks to depositFor().

M-3

RestrictedNORI.revokeUnreleasedTokens() can revoke tokens for unreleased Removals and/or amounts

Topic
Input Validation
Impact
High
Likelihood
Medium

RestrictedNORI:revokeUnreleasedTokens() does not perform any validation that the project or release amount corresponds to Removals that were actually released. This allows for unreleased Removals, or amounts in excess of released amounts, to be revoked.

Remediations to Consider

Consider updating the design of RestrictedNORI.sol:revokeUnreleasedTokens() to be callable only from Removal.sol:release(), for only the specific Removal amount that was released.

Response by Nori
  • Revocation of restricted NORI is at the project level by design and it’s valid for release of carbon in one field to spill over and cause revocation of RestrictedNORI attributed to other removals in the project.
  • Resolution: To improve transparency around the reason for a revocation we added a removal ID to the revocation event to provide a stronger link between release of a removal and the subsequent revocation call. In the future we intend to further automate the connection between release, revocation and replacement of released removals attributed to a certificate.
M-4

Previously sold Removal can be re-listed and re-sold in the Market

Topic
Input Validation
Status
Impact
High
Likelihood
Medium

Removal.sol consign() does not validate the from argument, which allows previously sold Removals locked in a Certificate to be re-sold in the Market. This can divert funds from legitimately unsold market Removals by double-paying for re-listed Removals. The removal’s supplier will also be double paid.

Remediations to Consider

Consider adding validation to consign() to prevent re-listing Removals currently owned by the Certificate.

Note that Removals actively listed in the Market can also be re-consigned, but no ill-effects were found. For correctness, consider also validating that the from address is not the Market.

M-5

No boundary check for holdbackPercentage in mintBatch

Topic
Input Validation
Status
Impact
Medium
Likelihood
Medium

In Removal.sol’s mintBatch function, a holdbackPercentage > 100% can be passed and the affected removals can be listed in the market (via consign function). If so, a subsequent call to the Market.sol’s swap function would revert due to an underflow on line 811.

This could stop the whole market’s swap mechanism from functioning if the affected removal is selected as the next removal in the _allocateSupply or _allocateSupplySingleSupplier functions.

Remediations to Consider

Consider adding an upper limit of 100 for holdbackPercentage.

M-6

Selection of next supplier is not working as expected

Topic
Spec
Status
Impact
High
Likelihood
Medium

In Market.sol’s swap function, removals should be allocated and purchased from suppliers in a round-robin way. Meaning that the oldest removal of the 1st supplier being listed is used up first, then the oldest removal of the 2nd supplier is used up, and so on.

However, currently removals are allocated as follows: Once a removal is fully used up, the current supplier is incremented twice, resulting in skipping the next supplier in the list. Consider the following scenario:

  1. 3 suppliers get listed in Market.sol in the following order:
  • Supplier_A lists a removal with amount of 1 * 10**18
  • Supplier_B lists a removal with amount of 1 * 10**18
  • Supplier_C lists removal with amount of 1 * 10**18
  1. A user purchases an amount of 1.5 * 10**18 via the Market.sol’s swap function

  2. In the subsequent _allocateSupply function, the removal of Supplier_A will be fully used up taking 1 * 10 ** 18 and the remaining amount of 0.5 * 10 ** 18 will be taken from Supplier_C - completely leaving out Supplier_B.

See Market.sol lines 978-989 below. Note that the current supplier is incremented twice, once in removeActiveRemoval (which sets Supplier_B as current supplier) and once by calling incrementCurrentSupplierAddress (which sets Supplier_C as current supplier). This leads to skipping Supplier_B and taking removals from Supplier_C instead.

_removeActiveRemoval({   
  removalId: removalId,
  supplierAddress: _currentSupplierAddress
});
if (
  /**
  *  If the supplier is the only supplier remaining with supply, don't bother incrementing.
  */
_suppliers[_currentSupplierAddress].next != _currentSupplierAddress 
) {
  _incrementCurrentSupplierAddress();
}

Remediations to Consider

Consider to only call _incrementCurrentSupplierAddress() if the current supplier hasn’t been already incremented in the _removeActiveRemoval function.

L-1

Removal.release() does not release removal IDs that are owned by the consignor role

Topic
Use Cases
Status
Wont Do
Impact
Low
Likelihood
Low

When removal.release() is called, removals should first be burned from unlisted balances, second from listed balances and third from certificates. Unlisted balances can include removals that are owned by the supplier or owned by the consignor role.

However, when the release() function is called, removals are never burned from the consignor role. This means listed removals and removals owned by certificates can be burned before all unlisted removals are burned.

Response by Nori
  • The consigner role is intended to be used only during the initial migration. We plan to monitor it to make sure removals are not sitting there and will remove the consignment role and code in a future release once the migration is complete.
L-2

LockedNORI allowed to enter insolvent state for existent grants, creating potential shortfall and/or trust concerns

Topic
Input Validation
Status
Impact
High
Likelihood
Low

LockedNORI.sol offers the ability to createGrant() without also locking the associated totalAmount. This creates a potential avenue in which the locked BridgedPolygonNORI is capable of being insufficient to cover all extant grants, such that some grantees may not be able to withdraw upon vesting/unlocking.

Note that should this occur, the shortfall can be remedied via depositFor(), which reduces this from a Medium to a Low severity vulnerability.

Remediations to Consider

Consider enforcing that grant-holders’ BridgedPolygonNORI is sufficient to cover all grants at time of grant creation. For example, consider adding logic to createGrant() which also mints LockedNORI and transfers bpNORI to cover the totalAmount.

L-3

_purchaseAmounts in Certificate.sol can show a false amount that was actually purchased for the certificate

Topic
Input Validation
Status
Impact
Low
Likelihood
Medium

When migrating removals to the certificate contract, the consignor role can input a certificateAmount that is not equal to the total balance of all corresponding removal tokens included in that certificate. This can cause confusion when the underlying removals are released and the certificates need to be topped back up to their original amounts.

Remediations to Consider

Validate that the certificateAmount is the sum of all of the removalAmounts .

L-4

supportsInterface incorrectly implemented in Certificate.sol

Topic
Interoperability
Status
Impact
Medium
Likelihood
Low

Due to how inheritance in Solidity works, the supportsInterface function in Certificate.sol does not support ERC721 and ERC721Metadata IDs. Thus, any call to supportsInterface with ERC721 interface ID (0x80ac58cd) or ERC721Metadata interface ID (0x5b5e139f), returns false.

Remediations to Consider

Consider to manually add those interface IDs:

function supportsInterface(bytes4 interfaceId)
  public
  view
  override(
    AccessControlEnumerableUpgradeable,
    ERC721AUpgradeable,
    IERC721AUpgradeable
  )
  returns (bool)
{
  return super.supportsInterface({interfaceId: interfaceId})
  || interfaceId == 0x80ac58cd || interfaceId == 0x5b5e139f;  
}
Q-1

Protocol users cannot readily verify that migrate() applied the correct ceritificateAmount for the supplied Removal amounts

Topic
Code Quality
Status
Quality Impact
Medium

The process which begins with Removal.migrate() ends with a call to Certificate._receiveRemovalBatch(), which only stores certificateAmount on chain, and only emits removalAmounts. Users cannot readily validate that both values are correct.

Consider adding certificateAmount to the ReceiveRemovalBatch event, or alternatively calculating certificateAmount on-chain.

Q-2

Eliminate cyclic dependencies by changing types or utilizing interfaces

Topic
Code Quality
Status
Quality Impact
Medium

Cyclic dependencies make it complex to reason about contract relationships and dependencies; and can also prohibit certain analysis and deployment tooling from functioning (e.g. npx hardhat flatten can fail due to cyclic dependency).

Some examples of cyclic dependencies found:

  • Removal.sol ←→ RestrictedNORI.sol (through Market.sol)
  • Certificate.sol ←→ Removal.sol (through Market.sol)

In some cases, only the dependency address is used (e.g. Certificate.sol only depends on the Removal address): in these cases, consider removing the contract-typed dependency and replacing it with an address type.

In other cases a restricted set of functions are used (e.g. RestrictedNORI.sol depends upon Removal.getProjectId()). Consider implementing an interface for the depended-upon functions, and replacing contract dependencies with interface dependencies.

Q-3

Improve readability of Market.sol function definitions

Topic
Code Quality
Status
Quality Impact
Medium

The functions _allocateSupply() and _allocateSupplySingleSupplier() specify returns() statements that do not include variable names. For example:

returns (
  uint256,
  uint256[] memory,
  uint256[] memory,
  address[] memory
)

The lack of variable names within the returns() portion of the function definition impairs code readability. Consider adding variable returns() names for both functions.

Q-4

Minimize unchecked{} scope to reduce risk of introducing vulnerabilities

Topic
Code Quality
Status
Wont Do
Quality Impact
Low

There are currently seven location in RestrictedNORI.sol where an unchecked{} block is used, which all follow a similar pattern:

// Skip overflow check as for loop is indexed starting at zero.
unchecked {
  for (uint256 i = 0; i < ...bounding value...; ++i) {
    ...loop logic...
  }
}

This explicitly leaves all loop logic unchecked, whereas the comment suggests the intention is to leave only ++i unchecked.

Fortunately, unchecked does not extend into library calls made from within an unchecked{} block. This prevented a High vulnerability which would have allowed a Supplier to withdraw all of their RestrictedNORI as soon as only 1 token was withdrawable. See the Validation section below.

As such, this code is presently only vulnerable to being brittle: the conditions for a High vulnerability have been created by an unchecked{} block, but are avoided by side-effects: Library code called from an unchecked{} block is itself checked. Should a future refactor pull the vulnerable logic out of the library and into RestrictedNORI.sol, the vulnerability will surface. The Validation section below emulates the effects of such a refactor, and illustrates the High vulnerability that would result.

To reduce resilience against these risks, consider minimizing the scope of unchecked{} blocks to the smallest amount possible. For example, the above pattern could be updated to:

for (uint256 i = 0; ...predicate...; ) {
  ...loop logic...

  // Skip overflow check as for loop is indexed starting at zero.
  unchecked{ ++i; }
}
Response by Nori

Won’t fix for now. Will keep an eye on it

Q-5

Incorrect @notice comment

Topic
Code Quality
Status
Quality Impact
Low

The @notice comment on LockedNORI.sol line 694 appears to be incorrect. Suspect copy-paste from the _vestedBalanceOf() @notice on line 704

Q-6

ArrayLib.sol: sum() allows overflow

Topic
Code Quality
Status
Quality Impact
Medium

The present implementation will not revert when the summation overflows. There are no related security vulnerabilities with present usages of sum().

Remediations to Consider

Consider updating the implementation to revert on overflow, to mitigate future introduction of vulnerabilities.

Q-7

ArrayLib.sol: slice() mutates the original array

Topic
Code Quality
Status
Quality Impact
Low

The implementation of slice() overwrites the original array’s value at index = to to the value of to-from, and returns that memory location as the ret array. This behavior has the following unexpected consequences:

  • When from == 0: The original array arr will be truncated in size to to-from elements
  • When from > 0: The original array arr will have the value at index from-1 overwritten to to-from

Due to these mutations, subsequent code that continues to use the original array after slice() is called on it may behave unpredictably. There are no related security vulnerabilities with present usages of slice().

Remediations to Consider

Consider updating the implementation to leave the original array un-mutated, to mitigate future introduction of vulnerabilities.

Alternatively, if truncation of the memory array is intended (given the present use of slice() and the circumstances outlined in G-1), consider renaming and refactoring slice() accordingly (e.g. truncate(uint256 newLength)) Care should be taken in the implementation such that the truncation length does not exceed the original length; and in the naming and documentation to make very clear that this has mutative effects.

Q-8

Documentation out of date

Topic
Code Quality
Status
Quality Impact
Low

NORI.sol’s documentation here states that minting and burning should be disabled.

However, NORI.sol inherits ERC20BurnableUpgradeable which contains a public burn()

function without any access control/reversion. When called, this will destroy the specified amount of tokens from the caller.

Since the Nori Team stated, “There is not currently any business case for preventing burning of that token so it sounds like the docs are out of date but the implementation is ok”, the documentation is considered out of date.

Remediations to Consider

Update the spec to not mention that burning is disabled to avoid confusion for users.

Q-9

Lack of testing on NORI.sol

Topic
Code Quality
Status
Quality Impact
Low

NORI.sol does not have any tests regarding its functionality.

Remediations to Consider

Create NORI.t.sol and NORI.test.ts files under the contracts/test folder to test its functionality.

G-1

Reduce array sizes in _allocateSupply() before summing and returning

Topic
Gas Optimization
Status
Gas Savings
High

_allocateSupply() will allocate memory arrays for ids , amounts, and with a length that matches the total number of Market listings, which may greatly exceed the number of allocated removals. _allocateSupplySingleSupplier() has similar logic. Each of these methods will sum over amounts before returning, which may largely be empty values when array length is much higher than allocated supply.

Consider moving the re-sizing logic present in _fulfullOrder() to each of these functions: prior to sum(amounts). This will save gas costs of summing over empty values.

Gas savings are substantial: in a test case of 10,000 total listed removals, calling swap() for 1 ether of removals incurs ~680,000 less gas.

G-2

LockedNori.sol _beforeTokenTransfer(): declare variables only when needed

Topic
Gas Optimization
Status
Gas Savings
Medium

Two variables are always set but not always used: ownerHasSufficientUnlockedBalance and ownerHasSufficientWrappedToken. See lines 678-688:

bool ownerHasSufficientUnlockedBalance = amount <= unlockedBalanceOf(from);
bool ownerHasSufficientWrappedToken = amount <= balanceOf(from);
if (isBurning && operatorIsNotSender && operatorIsGrantAdmin) {
  // Revocation
  require(balanceOf(from) >= amount, "lNORI: insufficient balance");
} else if (!isMinting) {
  // Withdrawal
  require(
    ownerHasSufficientUnlockedBalance && ownerHasSufficientWrappedToken,
    "lNORI: insufficient balance"
  );
}

Consider re-using ownerHasSufficientWrappedToken, which is always checked; and relying on boolean short-circuiting to only check vested balance when necessary. For example:

bool ownerHasSufficientWrappedToken = amount <= balanceOf(from);
if (isBurning && operatorIsNotSender && operatorIsGrantAdmin) {
  // Revocation
  require(ownerHasSufficientWrappedToken, "lNORI: insufficient balance");
} else if (!isMinting) {
  // Withdrawal
  require(
    ownerHasSufficientWrappedToken && amount <= unlockedBalanceOf(from),
    "lNORI: insufficient balance"
  );
}
G-3

LockedNori.sol _unlockedBalanceOf(): declare variables only when needed

Topic
Gas Optimization
Status
Gas Savings
Medium

vestedBalance is always set but not always utilized, see lines 746-759:

uint256 vestedBalance = _hasVestingSchedule(account)
  ? grant.vestingSchedule.availableAmount(atTime)
  : grant.grantAmount;
if (grant.exists) {
  balance =
    MathUpgradeable.min(
      MathUpgradeable.min(
        vestedBalance,
        grant.lockupSchedule.availableAmount(atTime)
      ),
      grant.grantAmount
    ) -
    grant.claimedAmount;
}

Consider moving the vestedBalance variable into the if() block.

G-4

whenNotPaused modifier is not needed on several external and public functions in Removal.sol, Certificate.sol, and RestrictedNORI.sol

Topic
Gas Optimization
Status
Gas Savings
Medium

In Removal.sol, mintBatch(), addBalance(), consign(), migrate(), release(), safeTransferFrom(), and safeBatchTransferFrom() functions do not need the whenNotPaused modifier because they all call beforeTokenTransfer() which also has the whenNotPaused modifier. This is also the case in Certificate.sol regarding the onERC1155BatchReceived() function and in RestrictedNORI.sol regarding the revokeUnreleasedTokens() function.

Remediations to Consider

Remove the whenNotPaused modifier on these functions to save on gas

Disclaimer

Macro makes no warranties, either express, implied, statutory, or otherwise, with respect to the services or deliverables provided in this report, and Macro specifically disclaims all implied warranties of merchantability, fitness for a particular purpose, noninfringement and those arising from a course of dealing, usage or trade with respect thereto, and all such warranties are hereby excluded to the fullest extent permitted by law.

Macro will not be liable for any lost profits, business, contracts, revenue, goodwill, production, anticipated savings, loss of data, or costs of procurement of substitute goods or services or for any claim or demand by any other party. In no event will Macro be liable for consequential, incidental, special, indirect, or exemplary damages arising out of this agreement or any work statement, however caused and (to the fullest extent permitted by law) under any theory of liability (including negligence), even if Macro has been advised of the possibility of such damages.

The scope of this report and review is limited to a review of only the code presented by the Nori team and only the source code Macro notes as being within the scope of Macro’s review within this report. This report does not include an audit of the deployment scripts used to deploy the Solidity contracts in the repository corresponding to this audit. Specifically, for the avoidance of doubt, this report does not constitute investment advice, is not intended to be relied upon as investment advice, is not an endorsement of this project or team, and it is not a guarantee as to the absolute security of the project. In this report you may through hypertext or other computer links, gain access to websites operated by persons other than Macro. Such hyperlinks are provided for your reference and convenience only, and are the exclusive responsibility of such websites’ owners. You agree that Macro is not responsible for the content or operation of such websites, and that Macro shall have no liability to your or any other person or entity for the use of third party websites. Macro assumes no responsibility for the use of third party software and shall have no liability whatsoever to any person or entity for the accuracy or completeness of any outcome generated by such software.