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

Patchwork A-2

Security Audit

Feb 1, 2024

Version 1.1

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 Dec 11, 2023 to Dec 19, 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 - 1 2
Low 4 - - 4
Code Quality 7 - - 7

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/IERC4906.sol

625e3580554a876b4849436745315468c7d09c020791d05ced350c67c17df5d2

src/IERC5192.sol

b71780f2006d0766390d1017292cc1b599114bb8d17f86cc4520149b010b49f5

src/IPatchwork1155Patch.sol

415bd774b9682d2ce13fe65ba6ce2a3f3060389b1555c23b3e5be45029341d3b

src/IPatchwork721.sol

9833f2927ec7a69b3067f0ce968095f6d8c1b7340f269b3b4fb6d769e7e97d09

src/IPatchworkAccountPatch.sol

8d87e3b2baf1adb2662b89520e4e24d66de7ef201022369ae5e4ba4e604cf143

src/IPatchworkAssignable.sol

ec5058ed3054e2a220ac64cfbbff2c7533782e396a943ccc79cfcdb03f118249

src/IPatchworkLiteRef.sol

b1301a7d3459bc9c0dd7a082305330c3823d662857dcbc45d961ca322f8d4406

src/IPatchworkMintable.sol

211e3a144162440cee8f1b51852ee0bf92d38dac3522c2f629897b3288fadcfb

src/IPatchworkMultiAssignable.sol

1a15dc9b73fa24041476f52455453f7b6cad3d17ce4e0b8cc40a1a5261c4c6e6

src/IPatchworkPatch.sol

66944f7df7fc0392fb7533baf99959b414448d271c9eaf1dd0b70cf92c61e693

src/IPatchworkProtocol.sol

9de6304edaed84b18488649ed50870d293e741ad5a2ede2c7c97bd4140298cb8

src/IPatchworkScoped.sol

f0b29a22fd78b0046215240835c6e9f43a10aa56e1d0b9ccdac3eb92fddd35a7

src/IPatchworkSingleAssignable.sol

801b6775d014424eee7e0e390431a2f117f90e640b7c31473d576e8c8273e97c

src/Patchwork1155Patch.sol

c2f14ec87df761136e621612ec4464e0cbae19c68e4c4d3111b30f3815200ad3

src/Patchwork721.sol

9f4e7340ac443dec0be78d7bf6b2626c5ff123a7fb00a11e0522c4306d12e933

src/PatchworkAccountPatch.sol

ee9c7c2964027886f80ba8f044d230797b8333441ad585f47051dbf6f30230ba

src/PatchworkFragmentMulti.sol

c2f51dbac6dc9c96027cb23c20a47025f6476d7edcdc010c96da603ca7c05aee

src/PatchworkFragmentSingle.sol

3a82011292196afe486791abdfd5f1cf993ba07af13059658050bf6bd7ce4301

src/PatchworkLiteRef.sol

ef45b47adedd613700746030b7432147e8c2d0f278cadc2dcdea0fc3d0b03907

src/PatchworkPatch.sol

d509dff138fbd1d60b944d2c1e3707ba9e58b21deedf5d4fb0b13748aef91371

src/PatchworkProtocol.sol

fc01291c747b3767a0411f0c25227842aee1a989f35c9bbf52e48954684ee1da

src/PatchworkUtils.sol

4fa529f39c7675f9f18544680029de7e5e84425e6a66e8499636c29dfe8d9e86

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

Incorrect state updates in PatchworkFragmentMulti.unassign()

Topic
Spec
Status
Impact
High
Likelihood
High

In the PatchworkFragmentMulti contract, on token unassign(), the corresponding storage variables are updated. Since corresponding structures include an array of assignments, a common pattern for removal is used where the last item replaces the removed element while the last element of the array is deleted.

However, concrete implementation is incorrect since it resets the pointer for the removed element, but it doesn’t update the pointer for the element that has been moved.

function unassign(uint256 ourTokenId, address target, uint256 targetTokenId) public virtual mustHaveTokenWriteAuth(ourTokenId) {
    AssignmentStorage storage store = _assignmentStorage[ourTokenId];
    (bool present, uint256 index, bytes32 targetHash) = _assignmentIndexOf(store, target, targetTokenId);
    if (!present) {
        revert IPatchworkProtocol.FragmentNotAssigned(address(this), ourTokenId);
    }
    Assignment[] storage assignments = store.assignments;
    if (assignments.length > 1) {
        // move the last element of the array into this index
        assignments[index] = assignments[assignments.length-1];
    }
    // shorten the array by 1
    assignments.pop();
    // delete the index
    delete store.index[targetHash];
}

In the code above, you may notice that store.index[of_last_item] has not been updated and will point to now invalid value. Consequent operations related to the moved token would have undefined behavior, and unassign() may fail due to index out-of-bound exception or even succeed by removing incorrect assignment.

Remediations to consider

  • Update unassign() implementation to properly update all relevant data variables related to elements affected.
H-2

Attacker may steal contract ETH balance due to incorrect accounting of assignFee in assignBatch()

Topic
Input Validation
Status
Impact
High
Likelihood
High

In the PatchworkProtocol, function _handleAssignFee() performs assign fee processing based on msg.value and configured fee for concrete fragment address. If the msg.value does not match the configured assignFee, processing will be halted, and a revert will be generated. If it matches and is greater than zero, protocol and scope balance will be proportionally increased based on the provided msg.value.

function _handleAssignFee(string memory scopeName, Scope storage scope, address fragmentAddress) private {
    uint256 assignFee = scope.assignFees[fragmentAddress];
    if (msg.value != assignFee) {
        revert IncorrectFeeAmount();
    }
    if (msg.value > 0) {
        uint256 assignBp;
        ProtocolFeeOverride storage feeOverride = _scopeFeeOverrides[scopeName];
        if (feeOverride.active) {
            assignBp = feeOverride.assignBp;
        } else {
            assignBp = _protocolFeeConfig.assignBp;
        }
        uint256 protocolFee = msg.value * assignBp / 10000;
        _protocolBalance += protocolFee;
        scope.balance += msg.value - protocolFee;
    }
}

While the above implementation works properly for the assign() and corresponding _doAssign() flow, it does not work properly when _handleAssignFee() is called within a loop, which is the case for flow which executes assignBatch(), _batchAssignCommon(), and _doAssign() as part of the inner loop.

In a problematic use-case scenario, the attacker (scope owner) may batch-assign N fragments by providing a msg.value amount for a single fragment. The same msg.value will be counted N times and correspondingly _protocolBalance and scope.balance would be incorrectly inflated for (N-1) * assignFee amount. The attacker may then withdraw the scope’s inflated balance, which would result in withdrawing assets belonging to other scopes that are managed through the PatchworkProtocol contract.

Remediations to consider

  • Do not use msg.value directly within the loop, instead use a specific variable initialized with msg.value and properly update it (reduce balance) on each inner loop iteration
M-1

Single locked fragment may get into an invalid state

Topic
Protocol design
Status
Wont Do
Impact
Medium
Likelihood
Medium

In the PatchworkFragmentSingle contract, token may get assigned by the PatchworkProtocol instance set as a manager and by the PatchworkFragmentSingle contract owner.

When the PatchworkProtocol manager calls assign(), it first enforces that the token is not locked by performing a corresponding check. However, when the caller is PatchworkFragmentSingle contract owner there is no corresponding check. As a result, the invariant that a fragment may get assigned only when it is not locked may be invalidated. As a side effect in this situation events may be emitted which do not correspond to the correct state.

Consider the following set of operations:

  • PatchworkFragmentSingle token owner locks it by calling setLocked(). As a result, parent Patchwork721.setLocked() is called, and Locked() event is emitted.
  • If assign() is invoked on PatchworkProtocol for the previously mentioned token, it will fail due to the token being locked.
  • However, if the PatchworkFragmentSingle contract owner calls directly assign() for the token, an assignment will be successful, token ownership will be transferred to the owner of the target token, and one more Locked() event will be emitted.
  • If unassign() is called for the token by the PatchworkProtocol or by PatchworkFragmentSingle contract owner it would succeed and it would emit Unlocked() event, even though token is still locked.

Remediations to consider

  • Remove the capability for PatchworkFragmentSingle contract owner to manage assignments directly and to change token ownership in that way, or
  • Enforce the same conditions in PatchworkFragmentSingle.assign() as PatchworkProtocol enforces, e.g., check that the token is not locked before performing the assignment.
Response by Patchwork

Works as intended. It is valid to be locked and then be assigned and continue to be locked.

M-2

Protocol owner and protocol bankers may update fees without delay

Topic
Trust Model
Status
Impact
High
Likelihood
Low

In the PatchworkProtocol, protocol owner and protocol bankers can update protocol and scope level fees anytime. In addition, they may increase fees to unexpected levels where scope owners are left with less revenue share than expected. In extreme cases, when the corresponding fee is set to 10000 or 100%, the scope will not accrue any revenue from corresponding actions.

Remediations to Consider

  • Consider using Timelock and time delay for important state changes.
M-3

Invalid fees may corrupt system operations

Topic
Input Validation
Status
Impact
High
Likelihood
Low

Protocol owner and protocol bankers may set various fees with values outside the expected range. For example, fees greater than 10000 would cause all important operations, where fees are expected to be charged, to fail due to the underflow.

Following operations would be affected due to reverts in _handleMintFee(), _handlePatchFee(), and _handleAssignFee().

  • mint()
  • mintBatch()
  • patch()
  • patch1155()
  • patchAccount()
  • assign()
  • assignBatch()

Moreover, the invalid fee may affect protocol or impact only a specific scope, depending on if it is a protocol-level fee or a fee override for a particular scope.

Remediations to Consider

  • Update setters setProtocolFeeConfig() and setScopeFeeOverride() to check that the corresponding max value for the fee is less than 10000bp or 100%, e.g., to 5000bp.
L-1

PatchworkFragmentSingle.unassign() works for tokens which are not assigned

Topic
Protocol Design
Status
Impact
Low
Likelihood
Low

In the PatchworkFragmentSingle, the contract owner may call unassign() for any token id (even non-existent). Since there are no checks that the token is assigned, unassign() for non-assigned tokens will be processed successfully and emit an Unlocked event.

Remediations to consider:

  • Consider adding check in unassign() that the assignment exists as it is performed in PatchworkProtocol, or
  • Remove the capability for the contract owner to call unassign() directly.
L-2

Missing event updates on important state changes

Topic
Events
Status
Impact
Low
Likelihood
High

In PatchworkProtocol, several functions with important state updates do not emit appropriate events. As a result, external actors may have a difficult time reacting to state changes in a timely manner.

  • setProtocolFeeConfig() - updates Protocol level fees which define revenue share split between scope owners and protocol.
  • setScopeFeeOverride() - updates scope level fees when they are meant to differ from the Protocol level fees. These define the revenue share split between the scope owner and protocol.
  • setPatchFee() - update scope level patch price.
  • setAssignFee() - update scope level assign price.

Remediations to Consider

  • Update the corresponding functions above to emit appropriate events.
L-3

getTokenIdForOriginalAccount and getTokenIdForOriginal1155 have unreliable output

Topic
Spec
Status
Impact
Low
Likelihood
High

In the PatchworkAccountPatch and Patchwork1155Patch contracts, getTokenIdForOriginalAccount() and getTokenIdForOriginal1155() are implemented to return value from the _patchedAddressesRev mapping variable.

However, the result may be correct tokenId or 0 depending on if withReverse parameter was true when the corresponding _storePatch() internal function was called.

Since both of these functions are external functions, their output would not be consistent and appropriate for others to rely upon it.

Remediations to consider

  • Consider updating _storePatch() to always set corresponding _patchedAddressesRev mapping entry and not to be optional as it is now.
L-4

redactReferenceAddress() and unredactReferenceAddress() may process invalid ids

Topic
Input Validation
Status
Impact
Low
Likelihood
Low

In the PatchworkLiteRef contract, redactReferenceAddress() and unredactReferenceAddress() functions can be called for references that are not yet generated. In addition, these functions will be successfully processed even when called with an invalid id such as 0 and 255.

Remediations to consider

  • Consider updating the implementation of these two functions to check that the provided id argument is a valid input.
Q-1

PatchworkPatch._burn() does not reset _patchedTokenIds

Topic
Best practice
Status
Quality Impact
Low

In the PatchworkPatch contract, the _burn() function reset several variables with info related to the removed token. However, it does not reset the corresponding entry in _patchedTokenIds mapping.

Consider updating _burn() implementation and adding the following change.

delete _patchedTokenIds[tokenId];
Q-2

Unnecessary getScope() implementation overrides

Topic
Unnecessary code
Status
Quality Impact
Low

Following implementation overrides are unnecessary since they are identical to the parent implementation in Patchwork721.

  • Patchwork1155Patch.getScopeName()
  • PatchworkAccountPatch.getScopeName()
  • PatchworkFragmentMulti.getScopeName()
  • PatchworkFragmentSingle.getScopeName()
  • PatchworkPatch.getScopeName()

Consider removing the mentioned functions.

Q-3

mustBeManager modifier not used

Topic
Unnecessary code
Status
Quality Impact
Low

In Patchwork721, the mustBeManager() modifier is defined but never used.

Consider removing this modifier and allowing child contracts to implement corresponding functionality independently.

Q-4

Use constants instead of hardcoded values

Topic
Best practices
Status
Quality Impact
Low

In the PatchworkProtocol, in several functions, the literal value of 10000 is hardcoded and used for fee split calculation.

  • _handleMintFee()
  • _handlePatchFee()
  • _handleAssignFee()

Consider defining a constant and reusing it in mentioned functions.

Q-5

Redundant Assignment struct definition

Topic
Best practices
Status
Quality Impact
Low

Assignment struct is used in PatchworkFragmentSingle and PatchworkFragmentMulti. Both of these contracts implement the IPatchworkAssignable interface. However, these contracts define the same Assignment structure twice in different locations. PatchworkFragmentSingle defines within a contract, while PatchworkFragmentMulti contains a definition in IPatchworkMultiAssignable interface.

Consider moving the Assignment struct definition to IPatchworkAssignable and removing it from the PatchworkFragmentSingle and IPatchworkMultiAssignable.

Q-6

Events are missing indexed attribute

Topic
Best practices
Status
Quality Impact
Low

In the IERC5192.sol, Locked and Unlocked events are missing indexed attribute for tokenId parameter, which would help off-chain tracking and monitoring.

In the IERC4906.sol, MetadataUpdate and BatchMetadataUpdate events are missing indexed attribute for their corresponding arguments which would help off-chain tracking and monitoring.

Consider adding indexed attributes for the mentioned events.

Q-7

Natspec improvements

Topic
Documentation
Status
Quality Impact
Low

Consider updating documentation for the following items.

  • Patchwork1155Patch._storePatch incorrectly ordered withReverse and account arguments

  • PatchworkFragmentMulti has an incorrect @dev comment for the contract (copy/paste issue)

    @dev base implementation of a Single-relation Fragment is IPatchworkAssignable

  • Incorrect notice comments in PatchworkUtils for several functions

    1. toString16() - Converts uint64 raw data to a 16 character string. It accepts uint128 and not uint64.
    2. toString32() - Converts uint64 raw data to a 32 character string. It accepts uint256 and not uint64.
  • PatchworkProtocol._doUnassign() - has missing natspec comments for some of the parameters (direct, targetMetadataId)

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.