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

Farcaster A-3

Security Audit

November 2nd, 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 October 23, 2023 to October 27, 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
Low 3 1 - 2
Code Quality 7 2 - 5
Gas Optimization 2 - 1 1

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

The Farcaster protocol utilizes various privileged actors:


All roles are assumed to be trusted and to act in a reliable and good manner. A more detailed description of the different actors and their assumptions can be found in the documentation.

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

5a08eaf5d1948901a1f9edb29e9b9330f8f35cd4c6744a3ec514950d7d1480ab

./src/IdManager.sol

8de857fc3d286538c896ff584f3ca09dcbc73c8ff1a1e4447ca1168c34533e7a

./src/IdRegistry.sol

21bdaf29d0a04be726bf62cb558d636d08b0dfeb490927d27dd3e6f6fa59951c

./src/KeyManager.sol

e1f63b135890888e57cd7f583801e686e32feccb73d6a46f4889a2ff146c19c0

./src/KeyRegistry.sol

4dfb929e519a5544196617acae5a04a4ace13b6ce7046c9d1fa4e69e3bb4b84e

./src/RecoveryProxy.sol

f3bf4e7b4db850376616b206df1118bfadb4fb8240823b82db6c6637a2214a14

./src/StorageRegistry.sol

a2f3eea1d11e039386ebe4455773d639274d006ade2697155f30d560cee2852d

./src/interfaces/IBundler.sol

76c4e2e407258a7e6421e08d6aea10088233fde099613d255b4a57cfa5d280b5

./src/interfaces/IIdManager.sol

290597dbd4177e6fc9b6691e04364fb1b18735d5306bae3fb56b68a33d0fe6da

./src/interfaces/IIdRegistry.sol

acd0bb92da459db17eaf819a222d91d09f38cf02ea647db78a93915e253db314

./src/interfaces/IKeyManager.sol

4e2bf273892b79f274954fa3cac38b0d6edc6637bc7c4e26ddc5f7adcc46804b

./src/interfaces/IKeyRegistry.sol

ee91997aa3c84a26645377c211e6e6bf42ca9d41cea74e5b6771073e9292066a

./src/lib/Guardians.sol

6a5e97ea59ad9f16a5962da89342add2f2215d9d772cd73fac27121f88819d20

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

L-1

KeyManager price logic is not consistent with other modules

Topic
Improper Validation
Impact
Medium
Likelihood
Low

Note: This issue was initially discovered by the Farcaster team within the scoped PR.

Reference: KeyManager.sol#L134, StorageRegistry.sol#L504

The StorageRegistry price logic has a differentiated price use for the cases where there is a price change triggered in the same block. This allows to keep the price consistent across all transactions within that block since the price update will take effect in the following block and the prevEthUsdPrice value will be used.

However, the KeyManager price uses the StorageRegistry:ethUsdPrice variable, making the price for this potential edge case different from that it would be for the other fee calculations.

L-2

Validation of deadline property for Signature is inaccurate

Topic
Improper Validation
Status
Impact
Medium
Likelihood
Low

Reference: Signatures.sol#L22

Signatures._verifySig performs the following expiration check:

if (block.timestamp >= deadline) revert SignatureExpired();

At the same time, the comment for error InvalidSignature() says:

/// @dev Revert when the block.timestamp is ahead of the signature deadline.
error SignatureExpired();

The comment doesn’t align with the actual logic, as the above check also reverts when block.timestamp exactly matches the deadline.

Remediations to Consider

Consider changing the above check to block.timestamp > deadline instead of >=.

L-3

Storage can be rented to fid = 0

Topic
Input Validation
Status
Acknowledged
Impact
Medium
Likelihood
Low

Reference: StorageRegistry.sol#L431, StorageRegistry.sol#L456, StorageRegistry.sol#L645, StorageRegistry.sol#L656

In StorageRegistry.sol, the functions rent(), batchRent(), credit(), and batchCredit() allow renting storage to fid = 0. However, as fid = 0 is an invalid fid, it leads to sending ether to the contract without renting storage to any fid.

Remediations to Consider Consider adding checks for the above functions to not allow fid = 0.

Response by Farcaster

We don’t intend to update the StorageRegistry as part of these changes, but will validate this in the next version. For now there is limited impact beyond callers unintentionally sending ether to the protocol.

Q-1

Inaccurate documentation

Topic
Documentation
Status
Quality Impact
Low

Reference: docs.md?plain=1#L192

The documentation states:

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

However, this is not the case since adding keys via KeyManager.add and KeyManager.addFor is not restricted and can be called by anybody even in the trusted state.

Consider updating documentation to properly reflect behavior.

Response by Farcaster

Note that IdRegistry and KeyRegistry now are actually deployed in a paused state. Updated the docs to reflect the latest and remove references to trusted caller.

Q-2

Guardians not mentioned in the documentation

Topic
Documentation
Status
Quality Impact
Low

Reference: docs.md

The documentation well explains the different roles of the system and also mentions under which assumptions they are acting. Since the recent changes, the protocol supports an additional role called “Guardians”, who have the privilege to pause contracts and can only be added by the contract owner.

Consider adapting the documentation to also mention the Guardian role (this is especially relevant for the “Assumption” section)

Q-3

Unused imports

Topic
Unused Code
Status
Quality Impact
Low

Reference: IdRegistry.sol#L6, KeyRegistry.sol#L5

The following import is not required and can be removed from IdRegistry.sol and KeyRegistry.sol:

import {Pausable} from "openzeppelin/contracts/security/Pausable.sol";
Q-4

Signature for addFor is not bound to a specific fid

Topic
Signature Validation
Status
Addressed
Quality Impact
Medium

Reference: KeyManager.sol#L239

The TYPEHASH used by the KeyManager.addFor function is defined as follows:

bytes32 public constant ADD_TYPEHASH = keccak256(
    "Add(address owner,uint32 keyType,bytes key,uint8 metadataType,bytes metadata,uint256 nonce,uint256 deadline)"
);

Since there is no fid encoded in the TYPEHASH, the signature is not bound to a certain fid. This opens up the possibility for the following edge case:

  1. Alice owns fid_123
  2. She creates a signature sig1, allowing someone else to add a key on her behalf.
  3. Later, Alice swaps her fid with Bob and she now owns fid_456. Until this point, sig1 has not been executed.
  4. Since sig1 is not bound to a certain fid, the signature can still be used via KeyManager.addFor to add a key to fid_456.

The above scenario allows for adding a key to fid_456. However, Alice created a signature with the intention to authorize adding a key to fid_123.

Consider adding the fid to the TYPEHASH to prevent the above scenario.

Response by Farcaster

This is an intentional design tradeoff that makes it possible to register an FID and add a key in a single transaction, without first knowing a user’s FID. We accept that this has the consequence described in the finding, and users should interpret these actions as “add key to current FID.”

We added a partial mitigation by exposing a public useNonce function on all contracts that use nonce-protected EIP712 signatures. Users who want to protect against this scenario can bump their nonce to invalidate previous signatures.

Q-5

extraStorage could be implemented in the IdManager

Topic
Design
Status
Quality Impact
High

Reference: Bundler.sol#L97, IdManager.sol#L109

The current function flow for a Bundler:register() call, goes through the following steps:

  1. register in the IdManager to register a new fid and rent one unit of storage, required for registration.
  2. add in the KeyManager to add the signers passed in the call.
  3. rent in the StorageRegistry to rent any additional storage needed, passed in the extraStorage input parameter.

This could be improved by passing additional storage into the IdManager and adding it to the required initial unit of storage since the IdManager already interacts with the StorageRegistry contract to rent storage, simplifying the interactions of the Bundler.

Q-6

Bundler would need to be redeployed even if only one manager gets changed

Topic
Design
Status
Acknowledged
Quality Impact
Medium

Reference: Bundler.sol#L47 , Bundler.sol#L52

The v3.1.0 update aims to create a more flexible system that enables the registry owner to safely update the manager modules. However, due to the immutability of the Bundler's idManager and keyManager variables, any changes made to the managers in the registry would require the bundler to be redeployed.

Consider having setter functions inside the Bundler contract to allow updating these modules easily.

Response by Farcaster

We’re OK redeploying Bundler contracts if necessary, since it’s cheap to do so on Optimism, and prefer to minimize our own control over the contract dependencies.

Q-7

Add fid validation to the migration process

Topic
Design
Status
Acknowledged
Quality Impact
High

Reference: Bundler.sol#L144

It is paramount, that users being migrated have the same fid on the new contracts. The migration on-chain logic doesn’t ensure this and solely relies on passing the UserData[] array in correct order to the Bundler.trustedBatchRegister function so that fid matches with the migrated contract.

Consider adding the corresponding fids to UserData struct so that during migration the correct fid can be validated against the current contract in Bundler.trustedBatchRegister.

Response by Farcaster

We don’t want to do this onchain, because it introduces an additional dependency on the old contract, but we will include validation against the previous contract state in our offchain migration scripts.

G-1

whenNotPaused modifier can be removed from IdManager functions

Topic
Duplicate validation
Status
Wont Do
Gas Savings
Low

Reference: IdManager.sol#L104, IdManager.sol#L120, IdManager.sol#L134

In IdManager.sol, the functions register(), registerFor(), and trustedRegister() utilize the whenNotPaused modifier, but also internally call idRegistry.register which by itself uses its corresponding internal whenNotPaused modifier. This leads to unnecessary gas consumption as the same modifier logic is evaluated twice.

Consider removing the whenNotPaused modifier from above functions.

Response by Farcaster

Pausing the IdRegistry pauses registration, transfers, and recovery, but pausing the IdGateway only pauses registrations. In the event of an change to the IdGateway, we want the ability to pause the old gateway contract without having to pause the IdRegistry.

G-2

Bundler performs redundant logic for registers without extraStorage

Topic
Redundant execution
Status
Addressed
Gas Savings
Medium

Reference: Bundler.sol#L97, Bundler.sol#L104, StorageRegistry.sol#L504

For users that register their fid through the Bundler but do not want additional storage than the required for registration, the logic will still perform price checks and additional calls to the StorageRegistry.

Consider adding a differentiated logic when extraStorage == 0 to avoid unnecessary logic execution, which users will pay.

Response by Farcaster

Implemented as part of Q-5.

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.