Security Audit
June 27th, 2024
Version 1.0.0
Presented by 0xMacro
This document includes the results of the security audit for thirdweb's smart contract code as found in the section titled ‘Source Code’. The security audit was performed by the Macro security team from May 23, 2024 to June 5, 2024.
The purpose of this audit is to review the source code of certain thirdweb 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 |
---|---|---|---|---|
Medium | 7 | - | 1 | 6 |
Low | 10 | - | - | 10 |
Code Quality | 6 | - | - | 6 |
thirdweb 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:
3d914b73d719d5c3dd7dbb30b673aff4d32d7168
We audited the following contracts:
Contract | SHA256 |
---|---|
src/ModularCore.sol |
|
src/ModularExtension.sol |
|
src/core/token/ERC1155Core.sol |
|
src/core/token/ERC1155CoreInitializable.sol |
|
src/core/token/ERC20Core.sol |
|
src/core/token/ERC20CoreInitializable.sol |
|
src/core/token/ERC721Core.sol |
|
src/core/token/ERC721CoreInitializable.sol |
|
src/extension/token/metadata/BatchMetadataERC1155.sol |
|
src/extension/token/metadata/BatchMetadataERC721.sol |
|
src/extension/token/metadata/DelayedRevealBatchMetadataERC721.sol |
|
src/extension/token/metadata/OpenEditionMetadataERC1155.sol |
|
src/extension/token/metadata/OpenEditionMetadataERC721.sol |
|
src/extension/token/metadata/SimpleMetadataERC1155.sol |
|
src/extension/token/metadata/SimpleMetadataERC721.sol |
|
src/extension/token/minting/ClaimableERC1155.sol |
|
src/extension/token/minting/ClaimableERC20.sol |
|
src/extension/token/minting/ClaimableERC721.sol |
|
src/extension/token/minting/MintableERC1155.sol |
|
src/extension/token/minting/MintableERC20.sol |
|
src/extension/token/minting/MintableERC721.sol |
|
src/extension/token/royalty/RoyaltyERC1155.sol |
|
src/extension/token/royalty/RoyaltyERC721.sol |
|
src/extension/token/transferable/TransferableERC1155.sol |
|
src/extension/token/transferable/TransferableERC20.sol |
|
src/extension/token/transferable/TransferableERC721.sol |
|
src/callback/BeforeApproveCallbackERC20.sol |
|
src/callback/BeforeApproveCallbackERC721.sol |
|
src/callback/BeforeApproveForAllCallback.sol |
|
src/callback/BeforeBatchTransferCallbackERC1155.sol |
|
src/callback/BeforeBurnCallbackERC1155.sol |
|
src/callback/BeforeBurnCallbackERC20.sol |
|
src/callback/BeforeBurnCallbackERC721.sol |
|
src/callback/BeforeMintCallbackERC1155.sol |
|
src/callback/BeforeMintCallbackERC20.sol |
|
src/callback/BeforeMintCallbackERC721.sol |
|
src/callback/BeforeTransferCallbackERC1155.sol |
|
src/callback/BeforeTransferCallbackERC20.sol |
|
src/callback/BeforeTransferCallbackERC721.sol |
|
src/callback/OnTokenURICallback.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.
OPTIONAL
does revert when not implemented in extension
mint()
doesn’t follow the CEI pattern can lead to potential reentrancy
ClaimCondition
can cause the caller paying more than expected
supportsInterface()
issues
name
and symbol
properties are not correctly initiated
eip712Domain()
function as a fallback function, lead to the function unreachable
initialize()
function
tokenURI
should revert for invalid tokenIds
_validateClaimCondition()
returns an outdated condition
batchMint
for ERC1155Core
contract and safeMint
for ERC721Core
contract are currently not supported
requiredInterfaceIds
getAllMetadataBatches
function
payable
tags
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. |
DelayedRevealBatchMetadataERC721 utilizes symmetric encryption to encrypt/decrypt the token URIs, meaning that the same encryption key is used for encrypting and decrypting the URI. The process is the following:
MINTER_ROLE
) encrypts the URI off-chain by using the secret encryption key.uploadMetadata
and passes the encryptedURI
in the _data
param.reveal
with the proper encryption key, the encryptedURI
is decrypted and the revealed URI can be retrieved via tokenURI
.The above process is secure, as long as the encryption key can be kept secret and doesn’t get into the hand of a malicious actor. However, exactly this can happen when the minter calls either
encryptDecrypt
to encrypt the datagetRevealURI
for e.g. verification purposes.Even the calls are declared as pure
and view
function and don’t generate an actual transaction on the chain, they can still be potentially intercepted on the RPC node. If this happens, a malicious actor can gain knowledge of the secret to take advantage of revealing the encrypted URIs before they are actually revealed by the minter, and thereby defeating the whole purpose of the “delayed reveal” functionality.
Remediation to Consider
The data encryption process should be fully off-chain, allowing the encryptDecrypt
function to be changed to private
. Also, completely remove the public getRevealURI
function from the DelayedRevealBatchMetadataERC721 contract.
OPTIONAL
does revert when not implemented in extension
Callback functions can be marked by the core contract as either OPTIONAL or REQUIRED. Marking the callback as OPTIONAL means that the execution shouldn’t revert if the callback function isn’t implemented by the extension.
However, this is currently not the case, as trying to execute the callback via _executeCallbackFunction
reverts when an OPTIONAL callback is not implemented by the extension.
In ModularCore._executeCallbackFunction
, the following logic implemented to check for callbacks:
if (callbackFunction.implementation != address(0)) {
(success, returndata) = callbackFunction.implementation.delegatecall(_abiEncodedCalldata);
} else {
if (callbackMode == CallbackMode.REQUIRED) {
revert CallbackFunctionRequired();
}
}
if (!success) {
_revert(returndata, CallbackExecutionReverted.selector);
}
Reference: ModularCore.sol#L309-L319
The above logic works correctly for REQUIRED callbacks because the tx reverts with an CallbackFunctionRequired()
error when the callback is not implemented. However, it doesn’t work for OPTIONAL callbacks. In this case, the delegatecall
would return success=false
, causing the tx revert on the final !success
check.
Remediation to Consider
Modify the above logic to not revert on OPTIONAL callbacks.
mint()
doesn’t follow the CEI pattern can lead to potential reentrancy
mint()
function in ERC1155CoreInitializable
and ERC1155Core
contract:
function mint(address to, uint256 tokenId, uint256 value, bytes memory data) external payable {
_beforeMint(to, tokenId, value, data);
_mint(to, tokenId, value, "");
_totalSupply[tokenId] += value;
}
Reference: ERC1155Core.sol#L169-L174 , ERC1155Core.sol#L184-L189
Notice here that the _totalSupply
variable will get updated after _mint()
. While in the _mint()
function, it will call to to.onERC1155Received()
:
function _mint(address to, uint256 id, uint256 amount, bytes memory data) internal virtual {
... mint logic
// basically calling to `to.onERC1155Received()` if `to` is a contract
if (_hasCode(to)) _checkOnERC1155Received(address(0), to, id, amount, data);
}
Because _totalSupply
variable will get updated after _mint()
, it can potentially lead to reentrancy. Even though, in the current code there’s no harm for reentrancy to exploit this because _totalSupply
has no use case currently, it can be a huge risk when there are extensions that rely on _totalSupply
. Let’s take an example:
ERC1155Core
, which can limit the total supply of each tokenId to 5. To do that, the builder will have to develop beforeMintERC1155()
function in the extension contract. It will look something like this (this is a pseudo-code):function beforeMintERC1155(address _caller, address _to, uint256 _id, uint256 _quantity, bytes memory _data)
external
payable
virtual
returns (bytes memory result)
{
...
currentTotalSupply = ERC1155_CORE.totalSuppy(_id);
require(currentTotalSupply + _quantity <= 5, "Not allowed);
...
}
mint()
doesn’t follow the CEI pattern, so they can build a malicious contract that looks something like this (this is a pseudo-code):contract Malicious {
function mint() external {
// mint 4 token with tokenId = 999 to this address
ERC1155Core.mint((address(this), 999, 4, "");
}
function onERC1155Received(address,address,uint256,uint256,bytes) external returns(bytes4){
// check we minted 100 token with tokenId = 999 to this address
if (ERC1155.balanceOf(address(this, 999) < 100) {
// mint 4 token with tokenId = 999 to this address
ERC1155Core.mint((address(this), 999, 4, "");
}
return 0xf23a6e61;
}
}
Remediations to Consider
Make mint()
follow CEI pattern. Also while burn()
is not affected by reentrancy, it’s recommended to follow the CEI pattern
During the installation and uninstallation of an extension, if registerInstallationCallback
is set in the config, onInstall
and onUninstall
will be called respectively.
However, there could be extensions that only properly implement the onInstall
callback, but are missing an onUninstall
. In this case, the extension gets properly installed but is reverting when trying to get uninstalled in ModularCore._uninstallExtension
:
if (config.registerInstallationCallback) {
(bool success, bytes memory returndata) = _extension.call{value: msg.value}(
abi.encodeCall(IInstallationCallback.onUninstall, (msg.sender, _data))
);
if (!success) {
_revert(returndata, CallbackExecutionReverted.selector);
}
}
Reference:
As per above, the call to onUninstall
will fail for extensions not implementing an onUninstall
function and returns success=false
. Finally, it will revert on the last line with an CallbackExecutionReverted
error.
This can be especially problematic where an extension gets compromised and it is needed to get an extension uninstalled.
Remediation to Consider
Make onUninstall
optional and don’t revert if the call to onUninstall
returns false.
ClaimCondition
can cause the caller paying more than expected
In ClaimableERC20/721/1155, tokens can be claimed by either callers being part of the allowlist, or by callers specifying a properly signed ClaimRequest
. For allowlisted callers, the overall price to be paid is determined by the current pricePerUnit
and currency
address set in the ClaimConditions
. However, these values can be changed by an address holding the MINTER_ROLE
any time. This can lead to situations where an allowlisted caller pays substantially more than what they expected to pay. Lets consider the following scenario:
MINTER_ROLE
sets the initial pricePerUnit
to 1 ether and the currency
address to USDC.The above scenario only works when the caller approves a higher amount of tokens than the expected price to be paid. However, this is quite common for frontends to specify the max uint256 value as allowance, for better UX.
Remediation to Consider
Add the possibility for callers to specify the expected currency
and pricePerUnit
, so that the transaction reverts when there was a change in currency or price.
Extension contracts use the “Namespaced Storage Layout” to prevent collisions between different extensions as well as between extension and core contract.
However, this doesn’t protect against storage collisions between different version upgrades. It is important to layout the storage variables in a safe way to make them extendable for future versions. Specifically the layout of state variables used in /minting
and /royalty
cannot be considered safe for future upgrades.
Let’s take a look at the storage layout of ClaimableERC20.sol:
struct Data {
// sale config: primary sale recipient, and platform fee recipient + BPS.
ClaimableERC20.SaleConfig saleConfig;
// claim condition
ClaimableERC20.ClaimCondition claimCondition;
// UID => whether it has been used
mapping(bytes32 => bool) uidUsed;
}
saleConfig
is stored at offset 0 (from the defined namespace) and only takes one storage slot since it only contains one address field:
struct SaleConfig {
address primarySaleRecipient;
}
claimCondition
is stored at offset 1 and takes multiple storage slots for the different fields defined in ClaimCondition
.
Now lets assume we want to deploy a new version specifying an additional field platformFeeRecipient
in SaleConfig
.
struct SaleConfig {
address primarySaleRecipient;
address platformFeeRecipient; // added in V1
}
Above change would result in a storage collision as platformFeeRecipient
would overwrite the first field defined in claimConditions
, which can lead to severe vulnerabilities. Similar issues exist in the other extensions of /minting
and /royalty
.
Remediation to Consider
Adapt storage layout of /minting
and /royalty
extensions to make them extendable and safe for future upgrades.
supportsInterface()
issues
Adhering to ERC165 standard is especially important for architectures such as the present Modular Contract Framework to guarantee compatibility between different extensions and core contracts. There are a few issues being encountered by not adhering to the EIP165 specification.
ERC4906 (MetadataUpdate event): Some ERC721 extensions that change metadata are not complying to ERC4906. See L-6.
ERC2981 (NFT royalty): All the core contract’s supportsInterface
functions already comply to ERC2981 (returning true for interfaceId == 0x2a55205a
) even though they don’t support royalties by default. References:
→ Remediation: Remove 0x2a55205a
form the core contract’s supportsInterface
functions
ERC165 (supportsInterface): According to ERC165, supportsInterface
must return true for interfaceId == 0x01ffc9a7
, meaning the contract supports ERC165. This is currently not the case for the ERC20Core’s contracts. References:
→ Remediation: Add 0x01ffc9a7
to the supportsInterface
functions of above ERC20Core contracts.
ERC7572 (contractURI): All the core contracts comply to ERC7572 but doesn’t return true for ERC7525 interface in supportsInterface.
ERC173 (Ownership standard): The ModularCore contract complies to ERC173 but doesn’t return true for ERC173 interface in supportsInterface.
name
and symbol
properties are not correctly initiated
In ERC20Core and ERC1155Core, the name
and symbol
parameters are passed to the constructor and are meant to be assigned to their respective _name
and _symbol
state variables.
// Set contract metadata
_name = _name;
_symbol = _symbol;
Reference: ERC20Core.sol#L50-L51
However, as shown above, _name
is assigned to _name
instead of the passed parameter name
. Same goes for the symbol
parameter. As a result, name
and symbol
are not set on contract deployment and remain empty.
This could potentially hinder other protocols or frontends from integrating with Thirdweb’s ERC20 and ERC1155 tokens.
Remediation to Consider
Assign the passed parameter name
and symbol
to _name
and _symbol
, respectively.
In burn()
function in ERC20Core
and ERC20CoreInitializable
contract
function burn(address from, uint256 amount, bytes calldata data) external {
_beforeBurn(from, amount, data);
_spendAllowance(from, msg.sender, amount);
_burn(from, amount);
}
Reference: ERC20Core.sol#146-L151
Notice here that the function will always decrease from
's allowance to caller. Even when the caller is also from
, the function still decreases the allowance of themselves. As a consequence, in order to burn their own token, the caller must approve()
to themselves first.
Remediations to Consider
Check if the caller is from
, if it is, then the function should not decrease their allowance
eip712Domain()
function as a fallback function, lead to the function unreachable
In getExtensionConfig
function in minting/
contracts, it’s not including EIP712’s eip712Domain()
function as a fallback function.
As a result, eip712Domain()
function can’t be called from the core contract, since it is not included as an allowed fallback function.
Remediations to Consider
Consider including eip712Domain()
as a fallback function in extension’s getExtensionConfig()
function. More specifically, the extension contracts that need to be fixed will be ClaimableERC20
, ClaimableERC721
, ClaimableERC1155
, MintableERC20
, MintableERC721
, MintableERC1155
.
initialize()
function
In the core contracts, specifically ERC20
/721
/1155Core.sol
and ERC20
/721
/1155CoreInitializable.sol
, the initialize()
functions contains the following check
require(extensions.length == extensions.length);
References:
ERC20CoreInitializable.sol#L58
ERC721CoreInitializable.sol#L54
ERC1155CoreInitializable.sol#L60
Obviously it will always pass this requirement. The intention here should be validating extension’s
length and extensionInstallData’s
length
Remediations to Consider
Make the following change to affected contracts:
- require(extensions.length == extensions.length);
+ require(extensions.length == extensionInstallData.length);
In ModularCore
contract, funds can be received via:
receive() external payable {}
This will allow anyone sending native token directly to the contract. Currently there’s no use case for this feature. Because of that, when this contract holds native tokens, there’s no easy way to withdraw them. The protocol owner would need to install an extensions specifically for withdrawing those native tokens.
Remediations to Consider
Consider removing receive()
in ModularCore
contract.
ERC4906 is an extension of EIP721 and defines the events when the token’s metadata is changed. Specifically, when the metadata for a single token is changed it should emit:
event MetadataUpdate(uint256 _tokenId);
and changing metadata for a range of tokens should emit:
event BatchMetadataUpdate(uint256 _fromTokenId, uint256 _toTokenId);
The following /metadata
contracts comply to ERC4906:
MetadataUpdate
in setTokenURI
BatchMetadataUpdate
in setSharedMetadata
Whereas the following contracts don’t emit one of the above events and hence doesn’t comply to ERC4906:
Additionally, 0x49064906
should be added to config.supportedInterfaces
for all the above contracts as mentioned in the standard:
The supportsInterface method MUST return true when called with 0x49064906.
Remediations to Consider
Emit MetadataUpdate
or BatchMetadataUpdate
in DelayedRevealBatchMetadataERC721.uploadMetadata
and BatchMetadataERC721.uploadMetadata
functions. Additionally, consider adding 0x49064906
to config.supportedInterfaces
.
tokenURI
should revert for invalid tokenIds
As defined in ERC721 standard, calling tokenURI
with an invalid id should revert:
/// @notice A distinct Uniform Resource Identifier (URI) for a given asset.
/// @dev Throws if `_tokenId` is not a valid NFT. URIs are defined in RFC
/// 3986. The URI may point to a JSON file that conforms to the "ERC721
/// Metadata JSON Schema".
function tokenURI(uint256 _tokenId) external view returns (string);
ERC721Core.tokenURI
correctly reverts when used with the following extensions:
onTokenURI
reverts with BatchMetadataNoMetadataForTokenId
for invalid ids.onTokenURI
reverts with BatchMetadataNoMetadataForTokenId
for invalid ids.However, ERC721Core.tokenURI
doesn’t revert when used with the following extensions:
onTokenURI
returns an empty string for not existing ids.onTokenURI
returns a valid metadata JSON object.Remediations to Consider
Consider reverting onTokenURI
for SimpleMetadataERC721 and OpenEditionMetadataERC721 when provided id does not exist.
_validateClaimCondition()
returns an outdated condition
In ClaimERC20/721/1155 contracts, _validateClaimCondition
makes updates to claimCondition
but doesn’t return the changed conditions:
function _validateClaimCondition(address _recipient, uint256 _amount, bytes32[] memory _allowlistProof)
internal
returns (ClaimCondition memory condition)
{
condition = _claimableStorage().claimCondition;
...
_claimableStorage().claimCondition.availableSupply -= _amount;
}
Reference: ClaimableERC20.sol#L225
Notice here that _claimableStorage().claimCondition.availableSupply
will be reduced at the end of the function, but the return parameter condition
is assigned at the beginning. As a result, this function will return outdated condition
, more specifically, outdated condition.availableSupply
. In the current system, it will not affect anything but could potentially pose security risks in future updates.
Remediations to Consider
Make the assignment to the return parameter condition
at the end of the function.
In MintableERC20._distributeMintPrice()
function, it doesn’t check if msg.value
is zero when the currency is not the native token:
if (_currency == NATIVE_TOKEN_ADDRESS) {
if (msg.value != _price) {
revert MintableIncorrectNativeTokenSent();
}
SafeTransferLib.safeTransferETH(saleConfig.primarySaleRecipient, _price);
} else {
SafeTransferLib.safeTransferFrom(_currency, _owner, saleConfig.primarySaleRecipient, _price);
}
}
Reference: MintableERC20.sol#L233-L235
Consequently, the caller can potentially loose their native tokens when accidentally passed to the mint call.
Remediations to Consider
Consider adding a check msg.value == 0
when the currency is not the native token in the MintableERC20._distributeMintPrice()
function.
In ERC20/721/1155Core, functions that contain a _before
hook may be vulnerable to an reentrancy attack. Take ERC20Core.mint
for example:
function mint(address to, uint256 amount, bytes calldata data) external payable {
_beforeMint(to, amount, data);
_mint(to, amount);
}
Due to the nature of the _before
hook, _beforeMint
is called before the _mint
function, potentially making an external call before the state in _mint
is updated. This violates the CEI (Checks-Effects-Interact) pattern.
Remediation to Consider
Add a reentrance guard to the callback functions to avoid reentering into the core contract.
batchMint
for ERC1155Core
contract and safeMint
for ERC721Core
contract are currently not supported
In ERC721Core contracts, there’s only the mint()
function that can mint NFT for users. However, the mint()
function doesn’t check if the receiver is a contract and whether it can receive an NFT. This can lead to tokens being locked in the receiver contract.
In ERC1155Core contracts, there’s only the mint()
function that can mint a ERC1155 token to users. However, the mint()
function only supports minting a single tokenId. For the sake of usability and gas costs, a batchMint()
function could be provided to mint multiple tokenIds.
Remediations to Consider
Consider adding safeMint()
to ERC721Core
and ERC721CoreInitializable
contracts, and adding batchMint()
to ERC1155Core
and ERC1155CoreInitializable
contracts.
requiredInterfaceIds
Currently, an extension can only set a single requiredInterfaceId
function getExtensionConfig() external pure override returns (ExtensionConfig memory config) {
...
config.requiredInterfaceId = 0xd9b67a26; // ERC1155
}
Reference: ClaimableERC1155.sol#L166
It could be useful for an extension to specify multiple interface ids, as e.g. the extension needs the core to support both ERC721 and ERC2981 for compatibility. However, currently only one requiredInterfaceId
can be specified.
Remediations to Consider
To add more flexibility to the framework, consider adding support for specifying multiple requiredInterfaceIds
.
Core contracts such as ERC20CoreInitializable
, ERC721CoreInitializable
, ERC1155CoreInitializable
are meant to be deployed using the Clone pattern. This means that the user deploys a Clone proxy that points to the core contract’s implementation.
However, currently these core contracts can be deployed without being initialized and are also not protected against subsequent initialization.
This allows a malicious actor to take ownership of the core contract implementation by properly initializing the contract. As the owner, they can install a new extension containing selfdestruct logic.
It is important to note, since the Ethereum Dencun upgrade, calling selfdestruct doesn’t destroy the contract’s bytecode anymore as long as it is not triggered in the same transaction as contract creation, only funds held in the contract can be sent to the chosen target. Hence, with the new changes implemented in the Dencun upgrade, a malicious attacker cannot pose any severe harm on user deployments.
getAllMetadataBatches
function
MintableERC721
contract is basically a combination of ClaimableERC721
contract and BatchMetadataERC721
contract. However, MintableERC721
contract doesn’t contain BatchMetadataERC721
’s getAllMetadataBatches
function
Remediations to Consider
Consider adding getAllMetadataBatches()
function to MintableERC721
contract. This would also require to add it as an allowed callback function in MintableERC721.getExtensionConfig()
payable
tags
Applying the payable tags in core contracts is not consistent to applying payable
tags in the callback functions:
transferFrom
is payable but beforeTransferERC721
callback is notapprove
is payable but beforeApproveERC721
callback is notburn
is not payable but beforeBurnERC721
isburn
is not payable but beforeBurnERC20
callback isburn
is not payable but beforeBurnERC1155
callback isRemediation to Consider
Apply payable
only to function with a specific use case to receive native tokens.
In ModularCore._installExtension
L178, this
can be removed to avoid an external call. supportInterface()
would need to be made public.
- if (!this.supportsInterface(config.requiredInterfaceId)) {
+ if (!supportsInterface(config.requiredInterfaceId)) {
In ERC20Core._beforeTransfer
L228, data
param can be added to the callback for more flexibility.
In OpenEditionMetadataERC721.setSharedMetadata
L95-100, improve readability and gas consumption by making the following change:
- OpenEditionMetadataStorage.data().sharedMetadata = SharedMetadata({
- name: _metadata.name,
- description: _metadata.description,
- imageURI: _metadata.imageURI,
- animationURI: _metadata.animationURI
- });
+ OpenEditionMetadataStorage.data().sharedMetadata = _metadata;
In RoyaltyERC721.sol#L65 and RoyaltyERC1155.sol#L10 the following line can be removed to save gas:
function getExtensionConfig() external pure virtual override returns (ExtensionConfig memory config) {
- config.callbackFunctions = new CallbackFunction[](0);
...
}
In ClaimableERC20._validateClaimRequest
L299, it will revert with an underflow if _amount
is > availableSupply
. Consider reverting with a custom error when this is the case.
In ClaimableERC20._validateClaimRequest
L272, there is the following time range check:
if (block.timestamp < _req.startTimestamp || _req.endTimestamp <= block.timestamp) {
revert ClaimableRequestExpired();
}
However, the error ClaimableRequestExpired()
is misleading for the case of block.timestamp < _req.startTimestamp
, as in this case the request is not expired.
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 thirdweb 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.