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

thirdweb A-18

Security Audit

April 5th, 2024

Version 1.0.0

Presented by 0xMacro

Table of Contents

Introduction

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.

Overall Assessment

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.

Specification

Our understanding of the specification was based on the following sources:

Trust Model, Assumptions, and Accepted Risks (TMAAR)

Source Code

The following source code was reviewed during the audit:

We audited the following contracts within the contracts repository:

Contract SHA256
contracts/prebuilts/unaudited/airdrop/Airdrop.sol

74f76f1f1f05c7d083b0355dcfc4ef9f06a8c8f5a4f4533a00ca948a2a8ce244

We audited the following contracts within the contracts-pay-gateway repository:

Contract SHA256
src/PaymentsGateway.sol

1d378d4aa6360496829591233d446ae4fb5ee48fecfb3796074e394283e7e81c

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

M-1

Pulling tokens after initiating the token purchase potentially fails

Topic
Locked Funds
Status
Wont Do
Impact
High
Likelihood
Medium

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:

  1. initiateTokenPurchase is called with tokenAmount of 10_000 and forwardAddress of 0x123.
  2. At this stage, the PaymentsGateway token balance is 10_000
  3. Before the tokens are pulled by the forwarder contract, another initiateTokenPurchase is done with tokenAmount of 1_000 using the same forwardAddress of 0x123
  4. Now, the PaymentsGateway token balance is 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.

Response by thirdweb

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.

L-1

Airdropping tokens using push or signature-based methods can be griefed

Topic
Griefing
Status
Wont Do
Impact
Medium
Likelihood
Low

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

  • Explicitly pass in the amount of gas for the transfer, ensuring there is enough gas to allow the contract to execute normal operations, but not provide the entire gas of the transaction that can be used maliciously.
  • As a minimum, appropriate comments should be added to the susceptible airdrop functions, making above shortcoming clear to any caller.
Response by thirdweb

Gas estimation should be able to detect if a griefing recipient is in the list.

L-2

Completing the token purchase potentially locks native tokens

Topic
Locked Funds
Status
Impact
Medium
Likelihood
Low

In PaymentsGateway’s completeTokenPurchase, native tokens can get locked in the contract in the following ways:

  1. User accidentally sends msg.value > 0 when specifying an ERC20 token as tokenAddress. All msg.value passed will be locked.
  2. User sends 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

  • In case of tokenAddress is an ERC20 token, don’t allow msg.value > 0 to be passed
  • and in case of tokenAddress is the native token, make sure msg.value == tokenAmount
L-3

Initiating the token purchase doesn’t correctly account for fee-on-transfer tokens

Topic
Protocol Design
Status
Wont Do
Impact
High
Likelihood
Low

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.

Q-1

Missing natspec documentation

Topic
Documentation
Status
Quality Impact
Medium

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.

Q-2

Make processed mapping public

Topic
Interoperability
Quality Impact
Medium

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.

Q-3

Unused error definition

Topic
Documentation
Status
Quality Impact
Low

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.

Q-4

Use separate events for each airdrop type

Topic
Events
Status
Quality Impact
Low

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.

Q-5

Use safeTransferETH instead of low-level call

Topic
Consistency
Status
Quality Impact
Medium

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.

Q-6

Missing native token support for claim and signature-based airdrops

Topic
Protocol Design
Status
Quality Impact
Medium

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.

Response by thirdweb

We won’t be adding native token support for these.

Q-7

Contract signatures not supported

Topic
Protocol Design
Quality Impact
High

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.

G-1

Use verifyCalldata to verify Merkle tree

Topic
Gas Optimization
Status
Gas Savings
Low

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

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 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.