Security Audit
April 5th, 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 March 27, 2024 to March 29, 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 | 1 | - | 1 | - |
Low | 3 | - | 2 | 1 |
Code Quality | 7 | - | - | 7 |
Gas Optimization | 1 | - | - | 1 |
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:
b44b56363230afcfe5e9c8dbdccf654c54182467
f2db1bffe10cc8b455640d054940f5b7ef330208
We audited the following contracts within the contracts repository:
Contract | SHA256 |
---|---|
contracts/prebuilts/unaudited/airdrop/Airdrop.sol |
|
We audited the following contracts within the contracts-pay-gateway repository:
Contract | SHA256 |
---|---|
src/PaymentsGateway.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.
natspec
documentation
processed
mapping public
safeTransferETH
instead of low-level call
verifyCalldata
to verify Merkle tree
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 PaymentsGateway contract, when calling initiateTokenPurchase
with some ERC20 token set as tokenAddress
, the specified tokenAmount
is transferred to the PaymentsGateway contract and approval is given to the forwardAddress
:
SafeTransferLib.safeTransferFrom(req.tokenAddress, msg.sender, address(this), req.tokenAmount);
SafeTransferLib.safeApprove(req.tokenAddress, req.forwardAddress, req.tokenAmount);
The expected behavior of the forwarder contract is to pull the tokens from the PaymentsGateway contract in the same transaction as initiateTokenPurchase
. However, as this can’t be guaranteed for all forwarder contracts, the following issue can occur:
initiateTokenPurchase
is called with tokenAmount
of 10_000
and forwardAddress
of 0x123
. PaymentsGateway
token balance is 10_000
initiateTokenPurchase
is done with tokenAmount
of 1_000
using the same forwardAddress
of 0x123
11_000
However, the forwarder contract can now only pull 1_000
tokens instead of 11_000
, as this was the latest value that was approved. The issue occurs because on the safeApprove
call, the exact tokenAmount
is set without considering any previous approvals that have not yet been pulled.
Remediation to Consider
Increase the allowance by the tokenAmount
instead of setting it exactly to the tokenAmount
.
Looking at the forwarders we plan to use, the forwarder will pull the tokens in the same transaction. Also, it may add extra steps to keep track of allowance and set it to an increased value etc. We'll add this change if we use other forwarders in future that require this kind of setup.
In the Airdrop contract, the push and signature-based airdrop methods are susceptible to a griefing attack. When iterating over the array of provided _contents
, execution control is given to the recipient in certain situations, depending on the token standard and airdrop type being used:
airdropNativeToken
gives execution control to the recipient when executing the low-level .call
method.airdropERC20
gives execution control to the recipient by internally calling tokensReceived
hook when the provided token is compatible with ERC777 standard.airdropERC721
gives execution control to the recipient by internally calling onERC721Received
hook.airdropERC1155
gives execution control to the recipient by internally calling onERC1155Received
hook.As a result, a recipient contract can execute malicious code, such as consuming all the provided gas. If this occurs, calls could cost substantially more than expected or exceed the block gas limit, preventing calls from executing.
If this happens, the owner could always execute the airdrop again by omitting the malicious recipient for push-based airdrops. For signature-based airdrops, the owner would need to create a new signature excluding the malicious recipient.
Remediation to Consider
Gas estimation should be able to detect if a griefing recipient is in the list.
In PaymentsGateway’s completeTokenPurchase
, native tokens can get locked in the contract in the following ways:
msg.value > 0
when specifying an ERC20 token as tokenAddress
. All msg.value
passed will be locked.msg.value > tokenAmount
when specifying the native token as tokenAddress
. The excess amount (msg.value - tokenAmount
) will be locked.The owner of the contract could withdraw locked tokens and give back the overpaid amount to the user via the withdrawTo
function. However, it is recommended to prevent above scenario in the first place.
Remediation to Consider
tokenAddress
is an ERC20 token, don’t allow msg.value > 0
to be passedtokenAddress
is the native token, make sure msg.value == tokenAmount
In PaymentsGateway, the initiateTokenPurchase
function doesn’t correctly account for tokens that apply a transfer tax, which results in less value transferred than the tokenAmount
specified.
However, the tokenAmount
is approved to the forwardAddress
, allowing the forwarding contract to pull the full tokenAmount
, rather than the tokenAmount - fees
:
SafeTransferLib.safeTransferFrom(req.tokenAddress, msg.sender, address(this), req.tokenAmount);
SafeTransferLib.safeApprove(req.tokenAddress, req.forwardAddress, req.tokenAmount);
Remediation to Consider
To handle fee-on-transfer tokens correctly, calculate the contract’s balance before and after the safeTransferFrom call, and approve this difference instead of the full tokenAmount
.
natspec
documentation
The Airdrop contract is entirely missing appropriate natspec
documentation. Note that natspec
documentation is considered as good practice to make the intent of the function clear to users.
Remediation to Consider
Add natspec
documentation for all the functions, or at least for the external ones.
processed
mapping public
In the Airdrop contract, the processed
mapping is used to track already processed requests and prevents replay attacks. This information can help users or other contracts to check the status of a request using the request Id. However, because the processed
mapping is declared as private
, the processed status cannot be retrieved easily.
Similar issues exists with the processed
mapping in the PaymentsGateway contract.
Remediation to Consider
Make the processed
mapping public
.
The Airdrop contract declares the following error
error AirdropInvalidTokenAddress();
However, this error is not used anywhere in the code.
Remediation to Consider
Remove the above error declaration.
In the Airdrop contract, the same Airdrop
event is emitted for both the push-based as well as the signature-based airdrops, whereas for the claim-based airdrops, a separate AirdropClaimed
event is emitted.
Remediation to Consider
Use separate events for push and signature-based functions to make it clear which airdrop type was used when the event is emitted.
safeTransferETH
instead of low-level call
In the withdraw
function, SafeTransferLib.safeTransferETH
is used to transfer native tokens to the caller. However airdropNativeToken
, uses the low-level call directly:
(bool success, ) = _contents[i].recipient.call{ value: _contents[i].amount }("");
Remediation to Consider
For readability and consistency reason, use safeTransferETH
in the airdropNativeToken
function to transfer native tokens to the recipient, instead of using the low-level call.
In the Airdrop contract, all of the three types of airdrops naming push-based, signature-based and claim-based support airdropping ERC20, 721, and 1155 tokens.
However, in the current design, only the push-based airdrop supports native tokens.
Remediation to Consider
Add native token support for signature-based and claim-based airdrops.
We won’t be adding native token support for these.
With the current implementation, ECDSA.recover
is used for the signature-based airdrops to verify signatures. However, ECDSA.recover
only supports normal EOA signature, but doesn’t support contract signatures with EIP-1271 standard.
Remediation to Consider
Add EIP-1271 support to allow contract such as smart wallet creating and verifying signatures.
verifyCalldata
to verify Merkle tree
Solady’s MerkleTree library offers two ways to verify a provided leaf is in a Merkle tree, one where the proof is provided as memory with the regular verify()
function, and another that takes the proof as calldata
using verifyCalldata()
. When a user claims tokens and provides a proof, it is passed in as calldata
, since it is not altered during execution. calldata
is cheaper to read than memory, which leads to less gas consumed for users to verify their claims if the function verifyCalldata()
is used rather than verify()
.
Remediations to Consider
Replace the call to verify()
with verifyCalldata()
to save users gas when claiming
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.