Farcaster A-3

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.

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

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.

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.

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)

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";

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.

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.

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.

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.

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.

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.