Security Audit
Sep 27, 2023
Version 1.0.0
Presented by 0xMacro
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.
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.
Our understanding of the specification was based on the following sources:
The following source code was reviewed during the audit:
fae3910c9fc0e544ee700b949e0a699bfb0c55f6
8cb244465b53d4daa36fd1e453638f195fe82a06
Specifically, we audited the following contracts within this repository:
Contract | SHA256 |
---|---|
src/IERC5192.sol |
|
src/PatchworkNFTBase.sol |
|
src/PatchworkNFTInterface.sol |
|
src/PatchworkProtocol.sol |
|
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.
Click on an issue to jump to it, or scroll down to see them all.
PatchworkNFT:setFrozen()
applyTransfer()
may trigger unexpected onAssignedTransfer()
calls
PatchworkLiteRef
’s redacted functionality is unused
setLocked()
and setFrozen()
may emit unnecessary events
PatchworkLiteRef
implementation incompatible with ERC165 standard
batchAssignNFT()
PatchworkProtocol
PatchworkNFT
Selector
contract
locked()
in IPatchworkNFT
We quantify issues in three parts:
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. |
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
transferScopeOwnership()
to address(0)
, andPatchworkNFT:setFrozen()
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
setFrozen()
function, use _freezes[tokenId]
state instead of _locks[tokenId]
.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
tokenId
value is less than the type(uint56).max
and in that way explicitly enforce the current assumption.applyTransfer()
may trigger unexpected onAssignedTransfer()
calls
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
applyTransfer()
.PatchworkLiteRef
’s redacted functionality is unused
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
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
setLocked()
and setFrozen()
may emit unnecessary events
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_) {
...
}
PatchworkLiteRef
implementation incompatible with ERC165 standard
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:
super.supportsInterface(interfaceID)
to confirm support for ERC165's interfaceId
.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
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()
Thawed
****event might be enhanced by including the value of the nonce in its emissionIn 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.
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.
batchAssignNFT()
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:
batchAssignNFT()
functionality from the core protocol into a utility contract as it doesn’t represent the main system feature.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.
PatchworkProtocol
PatchworkNFT
PatchworkNFTInterface
PatchworkNFTInterfaceMeta
IPatchworkNFT
tokenId
vs _tokenId
)IPatchworkPatch
uint
and uint256
(e.g. in mintPatch()
declaration)PatchworkProtocol
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.
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.
PatchworkNFT
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.
Selector
contract
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.
locked()
in IPatchworkNFT
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.
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.