Reach out for an audit or to learn more about Macro
or Message on Telegram

Patchwork A-1

Security Audit

Sep 27, 2023

Version 1.0.0

Presented by 0xMacro

Table of Contents

Introduction

This document includes the results of the security audit for Patchwork's smart contract code as found in the section titled ‘Source Code’. The security audit was performed by the Macro security team from Aug 14, 2023 to Aug 21, 2023.

The purpose of this audit is to review the source code of certain Patchwork 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 2 - - 2
Medium 3 - - 3
Low 3 1 - 2
Code Quality 11 - - 11

Patchwork 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
src/IERC5192.sol

b71780f2006d0766390d1017292cc1b599114bb8d17f86cc4520149b010b49f5

src/PatchworkNFTBase.sol

698b29e16347b89243b711bb5cc7496a929e7635993ab14fe30de8329b792779

src/PatchworkNFTInterface.sol

441649858ec9c009715ed104773c78b8e770b28171792ff3ac2d54626a512f9c

src/PatchworkProtocol.sol

3f0174e1680b79bb64a54550c732e7018f81b83aad185f066ea9b4dd2d66d816

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

A malicious actor may preconfigure any scope with operators and whitelisted contracts under his control

Topic
Spec
Status
Impact
High
Likelihood
High

In the PatchworkProtocol contract, the scope is a main concept that defines a namespace with associated configuration settings valid for that namespace. If the scope’s owner is 0 address it is considered to be free and claimable. Anyone can claim the free scope using PatchworkProtocol:claimScope(). A successful claim will set the scope’s owner to the caller’s address while an unsuccessful one will revert due to the requested scope having an existing owner.

The expectation is that after being claimed, the scope is non-configured. Also, it is expected that the new scope owner will need to configure the scope to be used properly. The scope owner can set operators, whitelist contracts, enable or disable whitelist, and update several other configuration parameters. It can also transfer ownership to a different address using the transferScopeOwnership() function.

However, the expectation that the scope is non-configured after being claimed can be broken by the actions of the malicious actor. The current implementation allows an attacker to front-run scope claim. Within the same transaction, an attacker may configure it with operators and whitelisted contracts under its own control. For this attack to succeed an attacker needs to perform at the end transferScopeOwnership() to address(0) making it free or claimable by the original requester.

An original requester will successfully claim a particular scope. However, this scope will have extra operators, whitelisted contracts, and configuration settings that the current scope owner may not be aware of. As a result, an attacker may plant a backdoor in any new scope. For example, with an operator under its control, an attacker may whitelist and delist contracts, create patches, assign, batch assign, and unassign NFTs without being authorized by the current scope owner.

Remediations to Consider

  • When scope is claimed it should not be possible to delete it or make it free ever again. Therefore, do not allow transferScopeOwnership() to address(0), and
  • Emit events in all the state-changing functions so it is easier to inspect state changes with off-chain monitoring tools.
H-2

Improper implementation of PatchworkNFT:setFrozen()

Topic
Spec
Status
Impact
High
Likelihood
High

In the PatchworkNFT contract, the token owner may freeze it using setFrozen(). When the token is frozen, it cannot be assigned, batch assigned, or unassigned as a frozen state status is checked when these operations are performed by the PatchworkProtocol.

However, the setFrozen() function incorrectly reads the state of the _locks[tokenId] instead of _freezes[tokenId]. It performs extra conditions and logic on this improper value. This breaks the logic and spec of the contract. For example, if the token has been previously locked, it will not become frozen even though the transaction will succeed without reverts.

function setFrozen(uint256 tokenId, bool frozen_) public virtual {
    require(msg.sender == ownerOf(tokenId), "not authorized");
    //Should read the _freezes[tokenId] state variable 
    bool _frozen = _locks[tokenId];
    if (!(_frozen && frozen_)) {
        if (frozen_) {
            _freezes[tokenId] = true;
            emit Frozen(tokenId);
        } else {
            _freezeNonces[tokenId]++;
            _freezes[tokenId] = false;
            emit Thawed(tokenId);
        }
    }
}

Remediations to Consider

  • In the setFrozen() function, use _freezes[tokenId] state instead of _locks[tokenId].
M-1

LiteRef implementation approach incorrect for token IDs that may be larger than uint56

Topic
Input Ranges
Status
Impact
High
Likelihood
Low

In the PatchworkLiteRef, the getLiteReference() function generates a lite reference ID which acts as an identifier of the assignment of a particular fragment token to a particular PatchworkLiteRef contract instance. The given function generates a uint64 lite reference by applying a bitwise OR operation between a uint8 refId (shifted left by 56 bits) and a uint256 tokenId. The implementation contains an implicit assumption that every provided fragment nft contract uses a sequentially incremented approach for tokenId identifiers. In addition, it assumes that the values of tokenId cannot be greater than type(uint56).max.

function getLiteReference(address addr, uint256 tokenId) public virtual view returns (uint64 referenceAddress) {
    uint8 refId = _referenceAddressIds[addr];
    if (refId == 0) {
        return 0;
    }
    return uint64(uint256(refId) << 56 | tokenId);
}

However, this assumption is not explicitly enforced. As a consequence, when tokenId values are larger than the expected max value, the overall integrity of the lite reference implementation will be affected as multiple different tokenId values may map to the same lite reference ID (due to silent overflow with uint64 downcast). This may affect the assignment and un-assignment of NFTs in dependent PatchworkProtocol functionality.

Remediations to Consider

  • Explicitly check that the tokenId value is less than the type(uint56).max and in that way explicitly enforce the current assumption.
M-2

Incorrect trust assumption in applyTransfer() may trigger unexpected onAssignedTransfer() calls

Topic
Trust Model
Status
Impact
Medium
Likelihood
Medium

In the PatchworkProtocol contract, the applyTransfer() function is responsible for keeping in sync state of dependent NFTs. It is meant to be called by the instances of the PatchworkNFT contract whenever one of the transfer function variants is invoked. However, applyTransfer() function is public and can be invoked by anyone.

The applyTransfer() function performs several checks depending on which interfaces the caller supports. For example:

  • if the caller supports fragment functionality it will check if it is assigned and revert the token transfer if it is,

  • if the caller supports patch functionality it will revert immediately as soulbound tokens transfers are not supported,

  • if the caller supports patchwork NFT functionality it will check if it is locked and revert the token transfer if it is,

  • if the caller supports patchwork lite references functionality, it will perform retrieval of all references from the caller and then iterate over returned references to perform _applyAssignedTransfer() for each, which will call onAssignedTransfer() on each contract defined by addresses[i] value.

    if (IERC165(nft).supportsInterface(IPATCHWORKLITEREF_INTERFACE)) {
        IPatchworkLiteRef liteRefNFT = IPatchworkLiteRef(nft);
        (address[] memory addresses, uint256[] memory tokenIds) = liteRefNFT.loadAllReferences(tokenId);
        for (uint i = 0; i < addresses.length; i++) {
            if (addresses[i] != address(0)) {
                _applyAssignedTransfer(addresses[i], from, to, tokenIds[i]);
            }
        }
    }
    

As you may notice from the above, the current implementation assumes that the caller always returns valid data. However, the caller may return fake references when liteRefNFT.loadAllReferences(tokenId) is invoked. As a result, PatchworkProtocol will on behalf of the malicious actor execute onAssignedTransfer() for provided tokens with from and to addresses defined by the caller/attacker.

PatchworkFragment:onAssignedTransfer() even though it is public allows only the manager or PatchworkProtocol contract instance to invoke it. However, in this case, this check will be ineffective, and unexpected token Transfer() events will be emitted.

Incorrect event emissions may adversely affect off-chain monitoring tools and dependent systems. This is especially important for a project such as Patchwork which is meant to be used and integrated with by many others.

Remediations to Consider

  • Consider updating implementation to perform verification that the provided reference is indeed one that exists within the system and is managed by the caller of applyTransfer().
M-3

PatchworkLiteRef’s redacted functionality is unused

Topic
Spec
Status
Impact
Medium
Likelihood
High

The PatchworkLiteRef contract contains functionality for redacting (or suspending) previously registered reference addresses. This should not affect the fragments that are already assigned, but it should prevent any future assignments from that fragment contract.

However, if a particular identifier of reference address is redacted within PatchworkLiteRef this does not have any impact on the current system operation as the redacted state is never used or enforced anywhere within the current system implementation.

Due to the above, the current redacted functionality is incomplete and unused. So, there should be an additional check performed when assigning NFTs to ensure that they weren’t redacted.

Remediations to Consider

  • Consider adding redacted state checks when assigning NFTs to ensure that they weren’t redacted, or
  • Consider removing redacted functionality if it is not necessary.
L-1

Long assignment chains may not be processable

Topic
Coding Standards
Status
Acknowledged
Impact
Medium
Likelihood
Medium

In the PatchworkProtocol there are several recursive functions that are used to either check the parent NFT’s frozen state during a transfer operation or to update the ownership tree.

  • PatchworkProtocol::_checkFrozen()
  • PatchworkProtocol::updateOwnershipTree()

When initiating a transaction, there's a predefined gas limit, representing the utmost gas you're prepared to consume. Transactions that overshoot this limit are reverted, forfeiting the gas used until that juncture. Additionally, every block in Ethereum has its own ceiling for gas consumption, restricting the gas expenditure for any single transaction. Deep recursions might risk exhausting a transaction's gas or risk making operations cost-prohibitive.

Remediations to Consider

  • Consider limiting the level of nested assignments and/or
  • Document current limits for end users to be aware of them.
L-2

setLocked() and setFrozen() may emit unnecessary events

Topic
Events
Status
Impact
Low
Likelihood
Medium

In the PatchworkNFT contract, functions setFrozen() and setLocked() update frozen and locked state of the token respectively. When the state is updated corresponding events are emitted. The implementation of both functions contains logic to prevent unnecessary operations if the current state is the same as the requested state.

However, due to the implementation error when the current state and requested state are both false, the system will emit unnecessary events and in the case of the setFrozen() function perform unnecessary increments of _freezeNonces[tokenId].

This happens because the if (!(_frozen && frozen_)) will always be true for the case where the values are false and false for old and new state as shown in the code snippet below:

function setFrozen(uint256 tokenId, bool frozen_) public virtual {
    require(msg.sender == ownerOf(tokenId), "not authorized");
    bool _frozen = _freezes[tokenId];
        //Duplicate calls possible for the case {false, false} as the result is `true`
    if (!(_frozen && frozen_)) {
            ...
    }
}

The same issue applies to setLocked() which mirrors the setFrozen() implementation.

Remediations to Consider

  • Unnecessary events and state updates may affect off-chain monitoring tools and dependent clients. Therefore, consider updating the if condition logic in setFrozen() and setLocked() to the following:

    if (_frozen != frozen_) {
        ...
    }
    
L-3

PatchworkLiteRef implementation incompatible with ERC165 standard

Topic
Coding Standards
Status
Impact
Low
Likelihood
Low

In the PatchworkLiteRef contract, the supportsInterface() function is implemented in the following way:

function supportsInterface(bytes4 interfaceID) public view virtual returns (bool) {
    return interfaceID == IPATCHWORKLITEREF_INTERFACE;  //PatchworkLiteReferenceInterface interface id            
}

However, concerning the supportsInterface() function, adherence to the ERC165 standard requires one of the following approaches:

  1. Inherit from ERC165 and utilize super.supportsInterface(interfaceID) to confirm support for ERC165's interfaceId.
  2. Explicitly return true when the provided interfaceID matches the ERC165 interfaceID = 0x01ffc9a7.

Current implementation does not implement any of these options and therefore is incompatible with ERC165.

Remediations to Consider

  • Update implementation to be compatible with the ERC165 standard.
Q-1

Missing events for important state changes

Topic
Events
Status
Quality Impact
Low

Crucial state-altering functions do not emit any events. As a result, these changes are not logged. We recommend adding events to these functions to ensure state changes are comprehensively logged and indexed. This will make them easily searchable and facilitate the identification of significant state modifications with the help of off-chain monitoring and tracking tools.

  • PatchworkProtocol
    • claimScope()
    • addOperator()
    • removeOperator()
    • setScopeRules()
    • addWhitelist()
    • removeWhitelist()
  • PatchworkNFT
    • setPermissions()
    • storePackedMetadataSlot()
    • The Thawed ****event might be enhanced by including the value of the nonce in its emission
Q-2

Missing indexed attributes for Frozen and Thawed events

Topic
Events
Status
Quality Impact
Low

In the IPatchworkNFT interface, Frozen() and Thawed() events are defined. However, these events do not declare tokenId to be indexed, which enables the ability to search the logs in off-chain monitoring and tracking tools.

/// @notice Emitted when the freeze status is changed to frozen.
/// @param tokenId The identifier for a token.
event Frozen(uint256 tokenId);

/// @notice Emitted when the locking status is changed to not frozen.
/// @param tokenId The identifier for a token.
event Thawed(uint256 tokenId);

Consider adding indexed attributes for tokenId in Frozen() and Thawed() events.

Q-3

Define visibility for contract variables explicitly

Topic
Best practices
Status
Quality Impact
Low

In both the PatchworkNFTBase.sol and PatchworkProtocol.sol state variables are declared without visibility modifier, making them implicitly and by default internal. Taking into account that all of the contracts in the PatchworkNFTBase.sol are meant to be inherited, consider defining visibility for contract variables explicitly to facilitate usage and integration of Patchwork contracts.

Q-4

Remove redundant functionality in batchAssignNFT()

Topic
Best practices
Status
Quality Impact
Low

In the PatchworkProtocol, the batchAssignNFT() function enables users to assign multiple NFT fragments to a target NFT in a single batch. It provides a better user experience for a relatively common use case. In addition, as expected it shares a lot of functionality with the assignNFT() function which handles the assignment of a single NFT fragment to a target NFT. However, this shared functionality is duplicated. As a result, contract code size is bigger, and code maintenance may be more error-prone.

Consider:

  • extracting common functionality into the helper internal method, or
  • (re)moving batchAssignNFT() functionality from the core protocol into a utility contract as it doesn’t represent the main system feature.
Q-5

Use custom errors

Topic
Best practices
Status
Quality Impact
Low

Throughout the codebase, errors are defined as strings in require expressions. While this was for a long time the only option, since version 0.8.4 Solidity supports the custom errors feature which provides a better developer and integration experience. In addition, the use of this feature reduces contract size and improves the gas efficiency of corresponding system operations.

Consider updating PatchworkProtocol and PatchworkNFTBase to report custom errors which will require their enumeration and consistent application.

Q-6

Improve Natspec documentation

Topic
Natspec
Status
Quality Impact
Low
  • PatchworkProtocol
    • Add comments for state variables and events
  • PatchworkNFT
    • Add comments for state variables and events
    • Add comments for functions, including their objectives, input parameters, and return types.
  • PatchworkNFTInterface
    • PatchworkNFTInterfaceMeta
      • Missing natspec comment for structs and enums
    • IPatchworkNFT
      • Missing natspec comment for a return value for several functions
      • Inconsistent naming of tokenId argument in different functions (tokenId vs _tokenId)
    • IPatchworkPatch
      • Inconsistent use of uint and uint256 (e.g. in mintPatch() declaration)
Q-7

Consider using modifiers for common checks in the PatchworkProtocol

Topic
Best practices
Status
Quality Impact
Low

In the PatchworkProtocol, the following checks are repeated in various functions.

  • require(msg.sender == s.owner, "not authorized")
  • require(msg.sender == s.owner || s.operators[msg.sender], "not authorized")
  • require(scope.owner != address(0), "Scope does not yet exist")

For enhanced maintenance and readability consider implementing these checks in function modifiers or internal helper methods.

Q-8

Use built-in interface identifiers instead of precalculated constants

Topic
Best practices
Status
Quality Impact
Low

In the PatchworkNFTInterface, four different constants are defined, which represent interface identifiers. These constants are used in both PatchworkNFTBase and PatchworkProtocol for detecting specific functionality support. However, this approach is error-prone as interface functionality may change, and constants may end up being incorrect.

Consider using a built-in interface identifier, for example type(IPatchworkNFT).interfaceId instead of IPATCHWORKNFT_INTERFACE constant.

Q-9

Unused string conversion utility functions in PatchworkNFT

Topic
Extra Code
Status
Quality Impact
Low

In the PatchworkNFT contract, the utility functions _toString8(), _toString16(), _toString32(), and trimUp() are unused. For improved modularity and easier updates in the future, we recommend relocating them to a dedicated utility library instead of being an integral part of core system contracts.

Q-10

Unnecessary import of the Selector contract

Topic
Extra Code
Status
Quality Impact
Low

In the PatchworkNFTInterface.sol file Selector contract is defined. This contract is a utility contract, which is exclusively used to calculate ERC165 interface identifiers and for testing purposes. Both PatchworkNFTBase and PatchworkProtocol import PatchworkNFTInterface. As a result, the Selector contract code unnecessarily becomes part of the deployed contracts.

Consider relocating the Selector contract to the tests directory and reclassifying it as either a utility or a library function.

Q-11

Unnecessary declaration of locked() in IPatchworkNFT

Topic
Extra Code
Status
Quality Impact
Low

In the IPatchworkNFT interface, a locked() function is declared. This declaration has the same function signature as the IERC5192:locked() function declared in the IERC5192 interface which IPatchworkNFT inherits from.

Consider removing the locked() function declaration from the IPatchworkNFT as it is unnecessary, and update the corresponding IPATCHWORKNFT_INTERFACE value.

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 Patchwork 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.