Security Audit
June 24th, 2022
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 June 1, 2022 to June 17, 2022.
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 |
---|---|---|---|---|
High | 2 | - | - | 2 |
Low | 7 | - | - | 7 |
Code Quality | 5 | - | 1 | 4 |
Gas Optimization | 5 | - | 1 | 4 |
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:
e33de553cfcdbcaa7c0a179756488b4e1238291a
f10d5433f004260ed80ca877e5427fb273e2f40c
e1c2115c31a8be5e1453820b144c3ade01460f9a
Specifically, we audited the following contracts as part of Multiwrap contract audit:
Contract | SHA256 |
---|---|
contracts/multiwrap/Multiwrap.sol |
|
contracts/feature/ContractMetadata.sol |
|
contracts/feature/Royalty.sol |
|
contracts/feature/Ownable.sol |
|
contracts/feature/Permissions.sol |
|
contracts/feature/PermissionsEnumerable.sol |
|
contracts/feature/TokenBundle.sol |
|
contracts/feature/TokenStore.sol |
|
contracts/lib/CurrencyTransferLib.sol |
|
contracts/openzeppelin-presets/metatx/ERC2771ContextUpgradeable.sol |
|
contracts/interfaces/IMultiwrap.sol |
|
contracts/feature/interface/IContractMetadata.sol |
|
contracts/feature/interface/IRoyalty.sol |
|
contracts/feature/interface/IOwnable.sol |
|
contracts/feature/interface/IPermissions.sol |
|
contracts/feature/interface/IPermissionsEnumerable.sol |
|
contracts/feature/interface/ITokenBundle.sol |
|
We audited the following contracts as part of DropERC1155 contract audit:
Contract | SHA256 |
---|---|
contracts/drop/DropERC1155.sol |
|
contracts/lib/FeeType.sol |
|
contracts/lib/MerkleProof.sol |
|
contracts/lib/CurrencyTransferLib.sol |
|
contracts/openzeppelin-presets/metatx/ERC2771ContextUpgradeable.sol |
|
contracts/interfaces/IThirdwebContract.sol |
|
contracts/feature/interface/IPlatformFee.sol |
|
contracts/feature/interface/IPrimarySale.sol |
|
contracts/feature/interface/IRoyalty.sol |
|
contracts/feature/interface/IOwnable.sol |
|
contracts/interfaces/ITWFee.sol |
|
contracts/interfaces/drop/IDropClaimCondition.sol |
|
contracts/interfaces/drop/IDropERC1155.sol |
|
We audited the following contracts as part of SignatureDrop contract audit:
Contract | SHA256 |
---|---|
contracts/signature-drop/SignatureDrop.sol |
|
contracts/feature/ContractMetadata.sol |
|
contracts/feature/PlatformFee.sol |
|
contracts/feature/PrimarySale.sol |
|
contracts/feature/Royalty.sol |
|
contracts/feature/DelayedReveal.sol |
|
contracts/feature/DropSinglePhase.sol |
|
contracts/feature/LazyMint.sol |
|
contracts/feature/Ownable.sol |
|
contracts/feature/Permissions.sol |
|
contracts/feature/PermissionsEnumerable.sol |
|
contracts/openzeppelin-presets/metatx/ERC2771ContextUpgradeable.sol |
|
contracts/lib/CurrencyTransferLib.sol |
|
contracts/feature/SignatureMintERC721Upgradeable.sol |
|
contracts/feature/interface/IClaimCondition.sol |
|
contracts/feature/interface/IContractMetadata.sol |
|
contracts/feature/interface/IDelayedReveal.sol |
|
contracts/feature/interface/IOwnable.sol |
|
contracts/feature/interface/IPermissions.sol |
|
contracts/feature/interface/IPermissionsEnumerable.sol |
|
contracts/feature/interface/IPlatformFee.sol |
|
contracts/feature/interface/IPrimarySale.sol |
|
contracts/feature/interface/IRoyalty.sol |
|
contracts/feature/interface/ISignatureMintERC721.sol |
|
contracts/feature/interface/IDropSinglePhase.sol |
|
contracts/feature/interface/ILazyMint.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.
renounceRole()
call can corrupt roleMembers state
supportsInterface()
implementation
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. |
Multiwrap.sol supports receiving ETH by auto-wrapping incoming ETH to WETH. It does this by converting native tokens in CurrencyTransferLib through interaction with external WETH contract. After wrapping, the Multiwrap contract holds on to the wrapped native tokens until an unwrap is requested.
However, when a user invokes unwrap()
for an asset with underlying ETH, it always reverts, because the WETH contract cannot transfer native tokens back to Multiwrap due to its missing receive
function. As a result, the user's ETH is permanently stuck in the WETH contract, and the user cannot retrieve back his assets.
Consider implementing the receive()
function in Multiwrap to fix this issue.
In SignatureDrop.sol, reveal()
is used to replace placeholder tokenBaseUri
for a particular batch with final tokenBaseUri
based on previously provided encrypted string. reveal()
is protected and callable by a user with privileged role MINTER. It uses and relies on the getRevealURI()
to retrieve decrypted final tokenBaseUri
. For a proper reveal()
, getRevealURI()
must not revert.
However, in DelayedReveal.sol, getRevealURI()
is a public function and can be called by anyone. Also, this function can only be executed once. Called with a bad input, its last line updates state to cause all followup executions to revert:
function getRevealURI(uint256 _batchId, bytes calldata _key) public returns (string memory revealedURI) {
bytes memory encryptedURI = encryptedBaseURI[_batchId];
require(encryptedURI.length != 0, "nothing to reveal.");
revealedURI = string(encryptDecrypt(encryptedURI, _key));
delete encryptedBaseURI[_batchId];
}
An attacker may simply invoke getRevealURI()
with any key to cause a permanently invalid contract state for a not yet revealed batch. This is due to encryptDecrypt()
not reverting even if an incorrect _key
is provided by the caller.
Consider changing getRevealURI()
visibility to internal. In addition, consider introducing an extra argument to getRevealURI()
, e.g. expectedRevealedURI
and corresponding guard condition to check if expectedRevealedURI
matches revealedURI
generated by encryptDecrypt
method. This additional check may prevent contract owner from intentionally or accidentally breaking their batch reveal when they provide an incorrect decryption key.
renounceRole()
call can corrupt roleMembers state
In Multiwrap.sol, an public invocation of PermissionsEnumberable#renounceRole()
with a valid role argument can corrupt state in the PermissionsEnumberable#roleMembers
variable for that particular role. Take the following example call trace:
PermissionsEnumerable#renounceRole(minter_role, Alice)
Permissions#renounceRole(minter_role, account)
Permissions#_revokeRole(minter_role, account)
PermissionsEnumerable#removeMember(minter_role, account)
And the following implementation of removeMember():
function _removeMember(bytes32 role, address account) internal {
uint256 idx = roleMembers[role].indexOf[account];
delete roleMembers[role].members[idx];
delete roleMembers[role].indexOf[account];
}
When _removeMember()
is called with a valid role and unknown account, idx
is 0
, causing the contract to remove an unrelated member in the following line. This results in a corrupted state.
Consider updating Permissions.sol#renounceRole
to check if the account actually has the role that is being renounced.
supportsInterface()
implementation
In Multiwrap.sol, the supportsInterface()
function overrides both ERC1155Receiver's and ERC721Upgradeable's implementations:
function supportsInterface(bytes4 interfaceId)
public
view
virtual
override(ERC1155Receiver, ERC721Upgradeable)
returns (bool)
{
return
super.supportsInterface(interfaceId) ||
interfaceId == type(IERC721Upgradeable).interfaceId ||
interfaceId == type(IERC2981Upgradeable).interfaceId;
}
Due to how multiple inheritance works in Solidity, calling super
will not invoke the supportsInterface()
implementations for both parent contracts. As a result, this contract will not be recognized as an ERC1155Receiver by external contracts, possibly blocking 3rd party integration.
Consider updating supportsInterface() to properly advertise ERC1155Receiver support like so:
function supportsInterface(bytes4 interfaceId)
public
view
virtual
override(ERC1155Receiver, ERC721Upgradeable)
returns (bool)
{
return
interfaceId == type(IERC2981Upgradeable).interfaceId ||
ERC1155Receiver.supportsInterface(interfaceId) ||
ERC721Upgradeable.supportsInterface(interfaceId);
}
In SignatureDrop.sol, the default contract admin can lazy mint a batch with 0 tokens by calling lazyMint()
. As a result, the internal identifier for the new empty batch becomes the same as the identifier for the previous batch. Due to this identifier overlap, followup actions targeting the new batch result in changes for the previous batch. This allows an admin to overwrite tokenBaseURI
for the previous batch maliciously or accidentally by calling reveal()
for new batch as depicted in the following test:
function test_delayedReveal_withNewLazyMintedEmptyBatch() public {
vm.startPrank(deployerSigner);
bytes memory encryptedURI = sigdrop.encryptDecrypt("ipfs://", "key");
sigdrop.lazyMint(100, "", encryptedURI);
sigdrop.reveal(0, "key");
string memory uri = sigdrop.tokenURI(1);
assertEq(uri, string(abi.encodePacked("ipfs://", "1")));
bytes memory newEncryptedURI = sigdrop.encryptDecrypt("ipfs://secret", "key");
sigdrop.lazyMint(0, "", newEncryptedURI);
sigdrop.reveal(1, "key");
// token uri for token 1 is overwritten and it shouldn't
string memory newUri = sigdrop.tokenURI(1);
assertEq(newUri, string(abi.encodePacked("ipfs://secret", "1")));
vm.stopPrank();
}
Consider adding a guard to prevent SignatureDrop#lazyMint
from being invoked with 0 _amount
.
Permissions.sol’s implementation allows granting the same role to an account multiple times. It also allows removing a role from an account that doesn't have that role. This may result in unexpected RoleGranted
and RoleRevoked
event emissions.
Consider adding guards in Permissions.sol to prevent granting the same role to a particular account, and to prevent removing a role from an account that doesn't actually have the target role.
In SignatureDrop.sol, a call to grantRole()
results in the PermissionsEnumerable#_addMember()
internal function being called two times. As a result, the roleMembers[role].members
storage variable contains unwanted duplicate records.
Consider updating PermissionsEnumerable#grantRole
to not call _addMember()
, since it will already be executed as part of downstream processing.
The SignatureDrop specification describes claimCondition.startTimestamp as follows:
The unix timestamp after which the claim condition applies. The same claim condition applies until the startTimestamp of the next claim condition.
Based on the above description, SignatureDrop users may create a claimCondition
to enable token claiming at a specific time in the future. However, in DropSinglePhase.sol's claim function, startTimestamp is not checked. This allows users to start claiming immediately, even if startTimestamp is set in the future.
Consider updating the implementation to check if startTimestamp condition has been satisfied or updating documentation related to startTimestamp to make it clear that it is not enforced.
Multiwrap.sol relies on CurrencyTransferLib#transferCurrencyWithWrapper()
for proper operation. In this method, msg.value
is used to check if necessary assets have been provided.
However, note that transferCurrencyWithWrapper()
is called within a loop. Although not an issue today, if the parent contract later supports holding ETH via an upgrade, the new functionality may be vulnerable to having assets drained from the contract.
Consider not relying on msg.value
directly in a library function which can be executed in a loop, and instead refactor code to execute necessary checks on a more higher/appropriate level.
In SignatureDrop's lazyMint()
, the TokensLazyMinted
event is emitted in following way:
emit TokensLazyMinted(startId, startId + _amount, _baseURIForTokens, _encryptedBaseURI);
DropERC721.sol another contract which has similar functionality emits this event in the following way. Notice difference in second argument.
emit TokensLazyMinted(startId, startId + _amount - 1, _baseURIForTokens, _encryptedBaseURI);
Consider updating TokensLazyMinted
event emission in SignatureDrop's lazyMint()
to match specification.
Upgradable contracts in the hierarchy of contracts need to have __gap variable in order for future changes not to break contract storage.
Contracts aren’t meant to be upgradeable and the missing __gap variable is intended.
Several events could benefit from indexing:
Missing more detail natspec comments for some of the features (see IClaimCondition.sol as a reference):
Visibility for following methods can be changed from public to external:
Wrap executes three loops, all for iterating tokens.
All of the above can be combined in one loop, saving gas costs. The same can be said for unwrap as well, instead of 2 loops, there can be one.
Not fixing, suggested optimization requires refactoring code across several levels of contract inheritance.
TokenBundle#_setBundle
has a code path for updating the bundle, which is unused in Multiwrap’s context. It's not only unused but it's also executed while creating a bundle. As a result, whenever this method is invoked an unnecessary condition is checked each time in the loop, increasing gas costs.
Consider creating two separate functions for create and update.
The following optimizations are done in CurrencyTransferLib:
transferCurrency()
and transferCurrencyWithWrapper()
)safeTransferERC20()
)The optimizations done are logically correct. But the issue is that cases when these checks are satisfied are very rare, and optimizing for them, though saves gas costs for these edge cases, increases the gas costs for all other use cases.
Consider removing these optimizations.
Reduce the length of string error messages to reduce contract size. Also consider using Solidity 0.8.4+ feature - Custom Errors .
In method PermissionsEnumerable#getRoleMember
, return early when a match is found instead of iterating through the whole array on each invocation.
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.