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

Farcaster A-1

Security Audit

August 18th, 2023

Version 1.0.0

Presented by 0xMacro

Table of Contents

Introduction

This document includes the results of the security audit for Farcaster's smart contract code as found in the section titled ‘Source Code’. The security audit was performed by the Macro security team from August 3, 2023 to August 9, 2023.

The purpose of this audit is to review the source code of certain Farcaster 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 2 - - 2
Low 5 2 - 3
Code Quality 10 3 - 7

Farcaster was quick to respond to these issues.

Specification

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

Source Code

The following source code was reviewed during the audit:

Specifically, we audited the following contracts within this repository:

Contract SHA256
./src/Bundler.sol

9c0636735061ec6d249428681ead7419b72981f3e8f81af60e3b0a6750d35cdd

./src/FnameResolver.sol

e5b907142cf42b146f8250f740fcfa671f0e02f8310cb2b336360836a7907953

./src/IdRegistry.sol

c99948c09e4c5301ba5091543656be300d9184e008acb0281b4b459b39672845

./src/KeyRegistry.sol

5a136d6114227436594e9d6803bcf6178bf6105728f3ea59603d91d0dac21b39

./src/StorageRegistry.sol

90eb4d71fc0391ae9900d4a85fb3632f313958185a6f8d44505f1cbeeca7bbca

./src/lib/Signatures.sol

db4f0878ee70a5c9229be76bf1309984763fa77bc4a9c96c4fe9d1970a34a42b

./src/lib/TransferHelper.sol

43af182279e1984145d649cb00a2c97560d4eba673d2b8abbd9fb4236d58fac1

./src/lib/TrustedCaller.sol

33508f2fbb94a1733f2c79e4da1308460be68dd46503b414d1330a82af8ccb40

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

Incorrect rentedUnits accounting in continuousCredit() function

Topic
Use Cases
Status
Impact
Spec Breaking
Likelihood
High

In the StorageRegistry contract, the contract operator may credit a sequence of fids with free storage units. Natspec documentation and implementation indicate that start and end arguments are inclusive range boundaries, meaning that fids equal to start and end should be granted free credits too.

The current implementation does indeed credit all fids in the mentioned range with free credits. However, due to an off-by-one error in calculating the length of the range, the calculated value of totalUnits is smaller by units amount than what it is supposed to be. As a result, the invariant that rentedUnits is always equal to the sum of all units rented ever will be invalidated. In addition, the check related to the maxUnits may be ineffective.

Remediations to Consider

Update len calculation to len = end - start + 1 and update for loop condition to i < len.

M-3

Incorrect _TRANSFER_TYPEHASH prevents all fid transfers

Topic
Use Cases
Status
Impact
Spec Breaking
Likelihood
High

In the IdRegistry contract, _TRANSFER_TYPEHASH is set to an incorrect value. The underlying event signature is incorrect for the first argument - instead of address from, it should be uint256 fid.

bytes32 internal constant _TRANSFER_TYPEHASH =
        keccak256("Transfer(address from,address to,uint256 nonce,uint256 deadline)");

As a result, _TRANSFER_TYPEHASH within the contract has an incorrect value, and consequently, the request digest, which _TRANSFER_TYPEHASH is a composite part of, will have an incorrect value. Due to that, _verifyTransferSig() execution will fail unless callers replicate the same error in generating _TRANSFER_TYPEHASH on their end.

Remediations to Consider

Update _TRANSFER_TYPEHASH with the expected signature for the arguments in use.

bytes32 internal constant _TRANSFER_TYPEHASH =
        keccak256("Transfer(uint256 fid,address to,uint256 nonce,uint256 deadline)");
L-1

KeyRegistry does not restrict public interactions in the trusted phase

Topic
Use Cases
Status
Acknowledged
Impact
Medium
Likelihood
Low

As per the specification:

The KeyRegistry is deployed in the trusted state where keys may not be registered by anyone except the owner.

However, KeyRegistry public functions add(), addFor(), remove(), and removeFor() are not being restricted by any access control.

Remediations to Consider

Consider using the whenNotTrusted() modifier for these functions to disallow users from registering and removing keys before the open registrable phase.

Response by Farcaster

This is by design. Registrations are intentionally restricted during the trusted phase, but if a user owns an FID, we want them to be able to add and remove keys even during the trusted phase. The meaningful authorization check here should be ensuring that the caller owns an FID.

The trusted caller’s special privilege in the context of this contract is the ability to add a key on behalf of an FID without owning that FID or providing a signature from the owner. Once the trusted phase ends, we revoke that privilege and the only way to add/remove a key is to own the associated FID or provide a signature from the owner.

The different meanings of “trustedOnly” between this contract and the Bundler/IdRegistry are a bit confusing. We’ll add an explanation to documentation.

L-2

Assets can get stuck in the Bundler contract

Topic
Use Cases
Status
Impact
Medium
Likelihood
Low

The Bundler contract implements a receive() function to allow overpayments to be sent and later refunded in the register() logic. However, assets sent incorrectly to the contract will get stuck in this contract without recovery.

Remediations to Consider

  • Consider implementing some logic in the receive() function to check whether the msg.sender is the StorageRent contract and revert to avoid users mistakenly sending funds to the contract, or
  • Remove the need for Bundler to store native assets by passing the original msg.sender in the storageRegistry.rent() call so that the storageRegistry can refund msg.sender directly.
L-3

In the StorageRegistry the owner can set an arbitrary fixedEthUsdPrice

Topic
Input Ranges
Status
Impact
Medium
Likelihood
Low

When calculating the price to rent storage units, the oracle price feeds are used to get the ETH price in USD. The price feed is acceptable if it is bound within the priceFeedMinAnswer and priceFeedMaxAnswer.

However, the owner of the contract can set a non-zero value for the fixedEthUsdPrice to override the price feeds. But the fixedEthUsdPrice is not checked to be within the acceptable min/max bounds.

Remediations to Consider

Consider checking whether the fixedEthUsdPrice is within the acceptable range ofpriceFeedMinAnswer and priceFeedMaxAnswer.

L-4

The KeyRegistry contract can be deployed without any constraints on thegracePeriod

Topic
Input Ranges
Status
Impact
Medium
Likelihood
Low

In the KeyRegistry contract, admins can make corrections to the migrated data during the gracePeriod.

In the documentation, the grace period is mentioned to be 24 hours. However, the owner can set an arbitrarily large value for the gracePeriod during the deployment of the contract.

If gracePeriod is arbitrarily large, the owner will be permitted to make changes well after the migration has been assumed to be completed and the hubs have switched over to this contract as the source of truth.

Remediations to Consider

Consider setting a limit of 24 hours on the gracePeriod in the constructor of the contract.

L-5

Upon resetting the fixedEthUsdPrice the users could potentially be charged a stale rent price

Topic
Use Cases
Status
Acknowledged
Impact
Low
Likelihood
Low

In the StorageRegistry contract, if the eth/usd price feed is unavailable, it’s not possible to rent storage units. For the system to operate in such emergencies, the contract owners could set a non-zero value to the fixedEthUsdPrice , which then acts as a substitute for the price feed.

When the price feed becomes operational again, depending upon the order of steps undertaken to switch back to the price feed, it’s possible that the users would be charged a stale rent price.

Consider the case, fixedEthUsdPrice is set to 0, and there was no recent calls to the refreshPrice().

IfrefreshPrice() were to be called in the block wherefixedEthUsdPrice was reset and users attempt to rent storage units. The users would be charged stale prices, which were valid before the feed was deactivated.

The primary reason for this is the following code in _ethUsdPrice() function:

/**
 *  We want price changes to take effect in the first block after the price
 *  refresh, rather than immediately, to keep the price from changing intra
 *  block. If we update the price in this block, use the previous price
 *  until the next block. Otherwise, use the latest price.
 */

return (lastPriceFeedUpdateBlock == block.number) ? prevEthUsdPrice : ethUsdPrice;

Remediations to Consider

  • Consider enforcing that a successful refreshPrice() call is performed at least one block before the fixedEthUsdPrice is reset, which would resolve the issue of the users being charged stale prices, OR
  • Remove the requirement that within the same block, users are charged the previously available rent prices. This requirement is not respected in the current context when switching from the price feed to a fixed price and vice versa.
Response by Farcaster

Since Chainlink price feeds have historically been pretty stable, we don’t want to add extra complexity to the contract to handle this unlikely edge case onchain, especially since it requires coordinating calls across multiple blocks. However, if we do take emergency action that disables and re-enables price feeds, we’ll take care to follow the suggestion here and call refreshPrice manually at least a block before calling setFixedEthUsdPrice.

Q-1

The Bundler contract could be independent and reusable

Topic
Protocol Design
Status
Acknowledged
Quality Impact
High

The Bundler contract serves as a wrapper contract to allow users and the trusted caller to register fid, keys for a specific fid, and rent storage in one transaction. Currently, this contract inherits from the TrustedCaller contract and has immutable variables for each of the contracts it interacts with.

Since the Bundler contract’s logic is simple and each of the counterparts performs proper access control and state sanity checks, this contract could be reusable for potential migrations and even more stateless.

Consider:

  • Having set functions for each contract address (IdRegistry, StorageRegistry, and KeyRegistry), allowing to update them in case of migration and reusing a deployed Bundler.
  • Not inheriting from TrustedCaller since each IdRegistry call has proper access control checks on msg.sender and would avoid potential different trustedOnly states for different contracts.

This would save logic execution, such as setting the trustedOnly state differently from their counterparts and avoid subsequent redeploys of the contract with the same logic and use case.

Response by Farcaster

We’d like to keep the Bundler as simple as possible, and we’d rather pay L2 gas to redeploy if necessary than add complexity to the contract.

Q-2

Bundler:register() does not check for zero storageUnits input

Topic
Input Validation
Status
Quality Impact
Medium

If a user attempts to register by passing zero storageUnits, the function call will revert while performing the external StorageRegistry.rent() call. Consider validating that the storageUnits input parameter is higher than 0 before making the external function call.

Q-3

The StorageRegistry contract should be able to pause and unpause external functions

Topic
Spec
Status
Quality Impact
High

During the process of upgrading the StorageRegistry contract, the new contract is deployed and meant to be paused so that storage cannot be rented. And is later unpaused once the various migration steps are complete.

However, there’s no straightforward way to pause and unpause the storage contract’s functions.

Consider inheriting the Pausable contract in StorageRegistry to implement pause and unpause capabilities on the external functions.

Q-4

Contracts can be left in different inconsistent states

Topic
Access control
Status
Acknowledged
Quality Impact
Medium

We can find the TrustedCaller states in each of the Bundler, IdRegistry, and KeyRegistry contracts. Since these are independent contracts, they could have different inconsistent states for trustedOnly and trustedCaller.

Additionally, for the Bundler to interact with both IdRegistry and KeyRegistry, its address should be set as the trustedCaller address for both contracts.

Consider using one source of truth for the access control restrictions, potentially in the IdRegistry, since both Bundler and KeyRegistry perform external calls to the IdRegistry.

Response by Farcaster

Since Warpcast is the only entity interacting with trusted functions, in practice we’ll switch the IdRegistry first.

Q-5

StorageRegistry:withdraw() doesn’t revert for zero amounts

Topic
Input Ranges
Status
Acknowledged
Quality Impact
Low

Consider adding logic to revert the transaction if the treasurer attempts to withdraw a zero amount.

Response by Farcaster

We accept the risk here. This function is only callable by a trusted role and we accept that we might waste gas if we try to withdraw a zero amount.

Q-6

Improve natspec documentation

Topic
NatSpec
Status
Quality Impact
High

Bundler:

  • Incorrect nat-spec comments for the keyRegistry variable. Replace Address of the StorageRegistry contract with Address of the KeyRegistry contract
  • Missing natspec for keyRegistry param in the constructor natspec
  • Natspec comment for the registration parameter of register() function has incorrect natspec with reference to from, which doesn't exist and should be replaced with recovery.
  • Natspec documentation for trustedRegister() is missing multiple params (scheme, key, metadata)

IdRegistry:

  • Error in natspec for Register event

    // Transfer signature below should be **Transfer(alice, bob, ...)**
    Two Register(alice, ..., ...) cannot emit unless a **Transfer(..., alice, bob)** emits in between, where bob != alice.
    
  • Missing return param natspec for multiple functions (register, registerFor, verifyFidSignature, etc.)

KeyRegistry:

  • Incorrect nat-spec comments for the removeFor() function.

  • Replace Add a key on behalf of another fid owner with Remove a key on behalf of another fid owner.

  • Remove potentially obsolete part of the natspec comment for the Migrated event -

    Emit an event when the admin calls migrateKeys. **Used to migrate Hubs from using
    off-chain signers to on-chain signers.**
    
  • Natspec comments for the Add, Remove, and AdminReset events contain a typo in the keccak wording. The extra "c" in kecca~~c~~k should be removed.

StorageRegistry:

  • Missing natspec for the return value of rent() function
  • Incorrect event invariant mentioned for the SetDeprecationTimestamp event needs to be deleted to avoid any confusion.
Q-7

Repetitive logic in theKeyRegistry contract could be converted to a function modifier

Topic
Best Practices
Status
Quality Impact
Medium

This following check is performed in several functions related to adding and removing keys.

uint256 fid = idRegistry.idOf(fidOwner);
if (fid == 0) revert Unauthorized();

Consider moving the repetitive logic to a function modifier for better readability.

Q-8

Price feed updates may require redeploying the StorageRegistry contract

Topic
Refactor
Status
Quality Impact
High

Consider extracting price-related functionality into a separate updatable external contract. In case the external price feed contract needs to be replaced, the StorageRegistry will not be affected, and costly migration would be prevented.

Q-9

Remove unnecessary imports

Topic
Best Practices
Status
Quality Impact
Low
  • The following imports are unnecessary in the TrustedCaller contract
    • ECDSA, EIP712, Nonces, Pausable
  • The following imports are unnecessary in the KeyRegistry contract
    • ECDSA
Q-10

Missing event for the successful fid recovery

Topic
Refactor
Status
Quality Impact
Medium

In the IdRegistry contract, the recover() function is responsible for performing recovery on behalf of the custody address by transferring the fid to a new address. When this happens, a Transfer event is emitted.

However, for off-chain monitoring tools, it is not easy to differentiate a recovery versus a normal transfer event.

Consider introducing a new event to differentiate between a fid recovery and a normal transfer.

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