Fan Fun - EVM Contracts - Fan Fun

In the claim() function, the contract transfers the creator's portion of decayed value using the following line:

(bool creatorTransferSuccess,) = creator.call{value: creatorAmount}("");

A creator using a smart contract wallet could upgrade or modify its implementation to intentionally revert any incoming native funds. This would cause the above call to always fail, forcing the entire claim transaction to revert and preventing all subscribers from receiving their claims. This risk is further amplified by the absence of an emergency withdrawal function in the Keys contract, leaving any stuck funds unrecoverable.

Consider implementing some of the following protections:

• Implement Fallback Mechanisms: Consider adding logic that allows subscribers or an authorized party (e.g., the contract owner) to redirect the creator’s share to a predefined fallback address if the transfer fails repeatedly.

• Adopt Pull Over Push Patterns: Transition from a push-based fund transfer approach to a pull-based mechanism (see Pull Over Push Pattern). This would let creators withdraw their share at their convenience, preventing a malicious creator's wallet from blocking the claim process.

• Integrate Emergency Withdrawal: Incorporate an emergency withdrawal or sweep function to recover native funds in cases where funds become irretrievably locked due to creator misbehavior.

• Reassess Creator Fund Transfers: Reevaluate the design of fund transfers in the claim function to decouple state updates from external calls, ensuring that a single malicious creator cannot block the entire claim process.

These measures would reduce the risk of a malicious creator obstructing the claim process and improve the overall resilience and reliability of the contract's fund management.

RISK ACCEPTED: The Fan.Fun team accepted the risk of this finding indicating "To handle the issue of a creator potentially blocking claims, we will ignore calling claim on this creator. We will monitor this off chain and blacklist creators who do this".

If the decay protocol fee percentage (decayProtocolFeePercent) is set to 0, the claim process will revert. This is due to the adjusted fee calculation in the claim function. In the code snippet below, if decayProtocolFeePercent is 0, then protocolFeeAmount becomes 0. However, the subsequent subtraction (protocolFeeAmount - 1) will underflow, causing the transaction to revert:

function claim(address creator, address[] calldata subscribers) public {
  ...
  // Calculate fees using decay-specific fee percentages (using basis points)
  uint256 protocolFeeAmount = (totalDecayedValue * decayProtocolFeePercent) / 10_000;
  uint256 subscriberFeeAmount = (totalDecayedValue * decaySubscriberFeePercent) / 10_000;
  uint256 creatorAmount = totalDecayedValue - protocolFeeAmount - subscriberFeeAmount;

  uint256 adjustedProtocolFee = protocolFeeAmount - 1;

• Enforce a Minimum Fee: Add input validation in the constructor and/or setter functions to ensure that decayProtocolFeePercent cannot be set to 0.

• Conditional Adjustment: Alternatively, modify the fee calculation logic to only subtract 1 when protocolFeeAmount is greater than 0. This will allow the fee to be 0 without causing an underflow.

• Review Fee Adjustment Logic: Revisit the rationale behind subtracting 1 from protocolFeeAmount to ensure it meets the intended economic model without introducing risks when fee percentages are low or zero.

Implementing one of these approaches will prevent unintended reverts during the claim process and ensure the contract behaves as expected even when a fee of 0% might be desired.

RISK ACCEPTED: The Fan.Fun team accepted the risk of this finding indicating "We acknowledge this issue. We will never set decayProtocolFeePercent to 0".

The claim() function distributes subscriber fees by iterating over a caller-supplied list of subscriber addresses and sending funds using a low-level call:

for (uint256 i = 0; i < subscribers.length; i++) {
  if (subscriberRemainingKeys[i] > 0) {
    uint256 subscriberFee = (subscriberFeeAmount * subscriberRemainingKeys[i]) / totalRemainingKeys;
    (bool subscriberTransferSuccess,) = subscribers[i].call{value: subscriberFee}("");

Because the caller provides the subscriber list, a malicious subscriber can potentially be excluded from the claim. This mitigates the risk if the caller carefully curates the list. However, if the list is compiled automatically or without thorough vetting, an address that always reverts on receiving ETH can cause the entire claim transaction to revert, leading to a denial-of-service (DoS) for the claim process.

While the current design allows the caller to exclude known malicious subscribers from the list, this relies on manual vigilance and may not be foolproof in automated systems.

To further mitigate this risk, it is recommended to adopt a pull over a push pattern for external fund transfers. Specifically, instead of transferring subscriber fees directly in the claim process, record the owed amounts in a mapping (e.g., pendingWithdrawals) and let subscribers withdraw their funds via a separate withdraw() function. This approach minimizes the impact of any single malicious subscriber and enhances overall contract resilience against DoS attacks.

RISK ACCEPTED: The Fan.Fun team accepted the risk of this finding indicating "To handle the issue of a subscriber potentially blocking claims, we will ignore calling claim on this subscriber. We will monitor this off chain and blacklist subscribers who do this. We are highly optimizing for gas savings, therefore we are opting for the push pattern instead of the pull pattern where we have to store amounts per user".

The Keys contract currently lacks an explicit withdraw or sweep function to recover native funds that become stuck in the contract. This design decision can lead to situations where excess funds remain trapped.

For example, if a user checks the price at X amount and then submits a buy order with X (even though the price drops to X-1 before execution) the extra funds are not refunded. This behavior creates potential financial inefficiencies and user dissatisfaction. This issue is related to the previously noted "Front-Running Risks: Potential DoS, Sandwich Attacks, and Slippage in Buy/Sell" finding, where the requirement for exact payment further exacerbates the risk of funds becoming stuck.

• Implement Refund Logic: Update the buy (and possibly sell) functions to automatically refund any excess funds if the executed price is lower than the amount sent.

• Consider Emergency Withdrawal Options: Although the primary design discourages a sweep function, evaluate the possibility of an optional, secure emergency withdrawal mechanism that allows users to recover funds accidentally sent or left over due to price fluctuations.

• Update Documentation: Clearly document any changes made to the refund mechanism and/or emergency withdrawal process so that users understand how to recover excess funds.

These modifications will help mitigate the risk of funds being irretrievably locked in the contract, improving overall user experience and aligning with best practices for handling native tokens.

SOLVED: The Fan.Fun team fixed this finding in commit 2a15092 by implementing a refund for excess payment in buyKeys() so funds are not stuck.

The Keys contract does not include a pause mechanism that would allow the contract owner to temporarily disable certain functions in the contract. A pause mechanism is a critical feature that can be used to prevent potential issues in the event of a bug or vulnerability being discovered in the contract. It can also be used to prevent malicious actors from exploiting the contract while a fix is being implemented.

Consider implementing a pause mechanism in the Keys contract to allow the contract owner to disable certain functions in the contract in case of an emergency or bug discovery.

RISK ACCEPTED: The Fan.Fun team accepted the risk of this finding indicating "We don't foresee a need to pause our system. We don't want the ability to pause our system".

The protocol implements an extensive privileged role with significant control over critical protocol parameters and functionality. The owner role in this contract can execute powerful administrative functions that could affect the entire protocol's operation and user funds.

function setFeeDestination(address _feeDestination) external onlyOwner {
function setProtocolFeePercent(uint256 _feePercent) external onlyOwner {
function setSubscriberFeePercent(uint256 _feePercent) external onlyOwner {
function setDecayProtocolFeePercent(uint256 _feePercent) external onlyOwner {
function setDecaySubscriberFeePercent(uint256 _feePercent) external onlyOwner {

Related notes:

• If the owner sets the protocolFeeDestination to an address that does not accept native funds, the buyKeys() for the whole protocol would fail.

• The design choice that creators can buy their own keys before unlocking buys could also be considered a centralization for creators.

It is recommended the following:

1. Adopt Multisig or DAO-Based Governance: Transition the owner role to a multi-signature wallet or a DAO/governance model to distribute control and reduce reliance on a single party.

2. Enhance Off-Chain Signer Security:

   • Use an institutional-grade Hardware Security Module (HSM) or a threshold-signature scheme (e.g., MPC) for the off-chain signer key.

   • Implement robust key management policies and regular key rotations to reduce the risk of permanent compromise.

3. On-Chain Validation of Critical Operations: Where feasible, shift critical or sensitive logic on-chain to reduce reliance on off-chain logic.

4. Emergency Procedures and Transparency:

   • Provide transparent documentation or a public dashboard to inform users of any centralized intervention (e.g., pausing the protocol).

   • Plan a contingency strategy (e.g., timelock for changes or a publicly broadcasted freeze period) to mitigate abrupt or unannounced governance decisions.

By distributing control and securing off-chain components, the protocol can significantly lower its centralization risk and foster greater trust among participants.

RISK ACCEPTED: The Fan.Fun team accepted the risk of this finding indicating "We accept the risks. We will follow best practices and use a multisig owner in production".

While it was verified that it is not possible to sell an amount of 0 keys, it was found that users can buy 0 keys and effectively update the lastPurchaseTimestamp value.

Run the following test function to verify that calling buyKeys() with an amount of 0 updates the lastPurchaseTimestamp of the subscriber.

function test_Hal_buy_ZeroAmount() public {
    // Allocate 100 ether to subscriber1
    vm.deal(subscriber1.addr, 100 ether);

    // Advance time by 1 day to ensure currentPeriod > 0 before the subscriber buys keys
    vm.warp(block.timestamp + 1 days);

    // Start acting as subscriber1
    vm.startPrank(subscriber1.addr);

    // Calculate the price to buy 0 keys using the same method as the contract
    (uint256 basePrice, uint256 protocolFee) = keys.getBuyPriceAfterFee(creator1.addr, 0);

    c.log("Initial totalSupplies: %s", keys.totalSupplies(creator1.addr));
    (uint256 balance, uint256 lastPurchaseTimestamp) = keys.subscriberInfo(creator1.addr, subscriber1.addr);
    c.log("Initial subscriber1 balance: %s and timestamp %s", balance, lastPurchaseTimestamp);

    // Buy 0 keys for creator1
    keys.buyKeys{value: basePrice + protocolFee}(creator1.addr, 0);

    c.log("After totalSupplies: %s", keys.totalSupplies(creator1.addr));
    (balance, lastPurchaseTimestamp) = keys.subscriberInfo(creator1.addr, subscriber1.addr);
    c.log("After subscriber1 balance: %s and timestamp %s", balance, lastPurchaseTimestamp);

    vm.stopPrank();
}

Consider adding a zero amount validation check in the buyKeys() function as found in sellKeys().

ACKNOWLEDGED: The Fan.Fun team acknowledged the risk of this finding indicating "The user can buy 0 keys, but it cannot be used to manipulate expiry times. Therefore, no change is needed".

It was identified that the Keys contract inherits from OpenZeppelin's Ownable library. Ownership of the contracts that are inherited from the Ownable module can be lost, as the ownership is transferred in a single-step process.

The address that the ownership is changed to should be verified to be active or willing to act as the owner. Ownable2Step is safer than Ownable for smart contracts because the owner cannot accidentally transfer smart contract ownership to a mistyped address. Rather than directly transferring to the new owner, the transfer only completes when the new owner accepts ownership.

To mitigate the risks associated with single-step ownership transitions and enhance contract security, it is recommended to adopt a two-step ownership transition mechanism, such as OpenZeppelin's Ownable2Step. This approach introduces an additional step in the ownership transfer process, requiring the new owner to accept ownership before the transition is finalized. The process typically involves the current owner calling a function to nominate a new owner, and the nominee then calling another function to accept ownership.

Implementing Ownable2Step provides several benefits:

1. Reduces Risk of Accidental Loss of Ownership: By requiring explicit acceptance of ownership, the risk of accidentally transferring ownership to an incorrect or zero address is significantly reduced.

2. Enhanced Security: It adds another layer of security by ensuring that the new owner is prepared and willing to take over the responsibilities associated with contract ownership.

3. Flexibility in Ownership Transitions: Allows for a smoother transition of ownership, as the nominee has the opportunity to prepare for the acceptance of their new role.

By adopting Ownable2Step, contract administrators can ensure a more secure and controlled process for transferring ownership, safeguarding against the risks associated with accidental or unauthorized ownership changes.

ACKNOWLEDGED: The Fan.Fun team acknowledged the risk of this finding indicating "We are okay with the risks here".

It was identified that the Keys contract inherits from OpenZeppelin's Ownable library. In the Ownable contracts, the renounceOwnership() function is used to renounce the Owner permission. Renouncing ownership before transferring would result in the contract having no owner, eliminating the ability to call privileged functions.

/**
 * @dev Leaves the contract without owner. It will not be possible to call
 * `onlyOwner` functions. Can only be called by the current owner.
 *
 * NOTE: Renouncing ownership will leave the contract without an owner,
 * thereby disabling any functionality that is only available to the owner.
 */
function renounceOwnership() public virtual onlyOwner {
    _transferOwnership(address(0));
}

It is recommended that the Owner cannot call renounceOwnership() without first transferring ownership to another address. In addition, if a multi-signature wallet is used, the call to the renounceOwnership() function should be confirmed for two or more users.

ACKNOWLEDGED: The Fan.Fun team acknowledged the risk of this finding indicating "We are okay with the risks here".

It is best practice to set the visibility of state variables and constants explicitly. In the KeysLibrary library, three constants did not have the visibility set:

uint256 constant A = 2_000_000_000_000_000_000; // 2.0 in fixed point
uint256 constant B = 150_000; // Multiplier
uint256 constant C = 1_000_000_000_000_000; // Base price

Consider explicitly setting the visibility of all state variables and constants. More specifically, the code could be updated to:

uint256 internal constant _A = 2_000_000_000_000_000_000; // 2.0 in fixed point
uint256 internal constant _B = 150_000; // Multiplier
uint256 internal constant _C = 1_000_000_000_000_000; // Base price

SOLVED: The Fan.Fun team fixed this finding in commit 5c65c25 by setting the visibility of all constants as recommended.

It has been observed that some functionalities are missing emitting events. Events are a method of informing the transaction initiator about the actions taken by the called function. It logs its emitted parameters in a specific log history, which can be accessed outside of the contract using some filter parameters. Events help non-contract tools to track changes, and events prevent users from being surprised by changes.

Instances in Keys.sol:

function setFeeDestination(address _feeDestination) external onlyOwner {
    protocolFeeDestination = _feeDestination;
}

function setProtocolFeePercent(uint256 _feePercent) external onlyOwner {
    protocolFeePercent = _feePercent;
}

function setSubscriberFeePercent(uint256 _feePercent) external onlyOwner {
    subscriberFeePercent = _feePercent;
}

function setDecayProtocolFeePercent(uint256 _feePercent) external onlyOwner {
    decayProtocolFeePercent = _feePercent;
}

function setDecaySubscriberFeePercent(uint256 _feePercent) external onlyOwner {
    decaySubscriberFeePercent = _feePercent;
}

This list is not exhaustive, and it is recommended to review the entire codebase to identify additional instances where event emissions may be beneficial. A thorough analysis should be conducted to determine whether adding event emissions aligns with the intended design and provides value in tracking state changes.

All functions updating important parameters should emit events.

SOLVED: The Fan.Fun team fixed this finding in commit aa06bf6 by adding event emissions to all setters as recommended.

In programming, "magic numbers" refer to the use of hard-coded numerical or string values directly in the code without clear indication of their purpose or origin. The use of magic numbers can lead to confusion and make the codebase more difficult to understand, maintain, and update.

Several instances of magic numbers were found in the provided contracts. These values lack meaningful context, making it unclear whether they are arbitrary, derived from a specific standard, or require future modification. Their presence increases the likelihood of errors during maintenance or updates.

• In Keys.sol:

decayProtocolFeePercent = 1000; // 10%
decaySubscriberFeePercent = 1000; // 10%
...
uint256 protocolFeeAmount = (totalDecayedValue * decayProtocolFeePercent) / 10_000;
uint256 subscriberFeeAmount = (totalDecayedValue * decaySubscriberFeePercent) / 10_000;
...
protocolFee = (basePrice * protocolFeePercent) / 10_000;

• In KeysLibrary.sol:

UD60x18 supplyFP = ud(supply * 1e18);
UD60x18 newSupplyFP = ud((supply + amount) * 1e18);
...
uint256 denominator = (A * B) / 1e18;
...
uint256 secondTerm = (C * amount) / 1e18;

This list is not exhaustive, and it is recommended to review the entire codebase to identify additional instances where magic numbers are used.

To improve code maintainability, readability, and reduce the risk of potential errors, it is recommended to replace magic numbers with well-defined constants. By using constants, developers can provide clear and descriptive names for specific values, making the code easier to understand and maintain. Additionally, updating the values becomes more straightforward, as changes can be made in a single location, reducing the risk of errors and inconsistencies. For large numbers, consider using scientific notation (e.g., 1e4).

SOLVED: The Fan.Fun team fixed this finding in commit 7c9a86b and 9beca85 by removing magic numbers from the code. Note: it is worth mentioning that scientific notations would further increase readability.

The project codebase contains several stylistic inconsistencies and deviations from Solidity best practices, which, while not directly impacting functionality, reduce code readability, maintainability, and adherence to standard conventions. Addressing these inconsistencies can enhance the clarity and professionalism of the code.

Examples:

• Constants not in UPPER_CASE: The periodDuration constant below is not in UPPER_CASE as recommended by the Solidity style guide.

uint256 public constant periodDuration = 1 days;

• Local State Variable Could Be A Constant: The denominator local state variable of the getTotalCost() function in KeysLibrary is always 300000, but it is declared as a calculation of three different constant values:

uint256 denominator = (A * B) / 1e18;

Consider implementing the style improvements highlighted above to enhance the readability and consistency of the project.

SOLVED: The Fan.Fun team fixed this finding in commit 6e55670 by improving the style as recommended.

Indexed event fields make the data more quickly accessible to off-chain tools that parse events, and add them to a special data structure known as "topics" instead of the data part of the log. A topic can only hold a single word (32 bytes) so if you use a reference type for an indexed argument, the Keccak-256 hash of the value is stored as a topic instead.

Each event can use up to three indexed fields. If there are fewer than three fields, all the fields can be indexed. It is important to note that each index field costs extra gas during emission, so it's not necessarily best to index the maximum allowed fields per event (three indexed fields).

This is specially recommended when gas usage is not particularly of concern for the emission of the events in question, and the benefits of querying those fields in an easier and straight-forward manner surpasses the downsides of gas usage increase.

Consider adding the indexed keyword when declaring events, considering the following example in IKeys.sol:

event Claimed(address indexed creator, uint256 keyAmount, uint256 baseAmount, uint256 protocolFeeAmount);

event Subscription(
  address indexed subscriber,
  address indexed creator,
  bool isSubscribe,
  uint256 keyAmount,
  uint256 hypeAmount,
  uint256 protocolHypeAmount,
  uint256 supply
);

ACKNOWLEDGED: The Fan.Fun team acknowledged this finding indicating "We have an indexer that is already storing and indexing all event data in its own database, regardless of whether the fields are marked as indexed. Therefore, in our case we don't see a benefit in using the indexed keyword. We have removed the indexed keyword on all events to save gas.". The indexed keyboards were removed in commit bdeda88.

Although much of the code under review adheres to a generally consistent NatSpec standard, there are notable gaps and inconsistencies in the documentation. For example, some functions in IKeys.sol lack any NatSpec annotations entirely, and there is inconsistent spacing (e.g., missing new lines between error declarations and subsequent NatSpec comments).

These documentation shortcomings can hinder a clear understanding of the contract logic, making it more challenging for external developers, users, and auditors to fully grasp the intended behavior, usage constraints, and potential edge cases.

Adopt a unified NatSpec documentation approach throughout the entire codebase. Specifically:

1. Function-Level Annotations: Provide full NatSpec annotations (@notice, @dev, @param, @return, etc.) for every function, detailing its purpose, parameter usage, and return data.

2. File-Level Headers: Add standardized titles and author tags at the beginning of all contract files, including helper and core contracts, to improve clarity and maintain consistency.

3. Comprehensive Coverage: Extend NatSpec documentation to cover all structs, errors, and interfaces. In particular, address missing annotations in files like IKeys.sol and ensure proper formatting (such as maintaining appropriate new lines between declarations) for better readability.

Implementing these recommendations will significantly enhance the clarity, maintainability, and auditability of the codebase.

SOLVED: The Fan.Fun team fixed this finding in commits 710372e and c7ed3a8 by improving the NatSpec documentation as recommended.

During the security assessment, it was identified that some functions in the smart contracts lack proper input validation, allowing critical parameters to be set to undesired or unrealistic values. This can lead to potential vulnerabilities, unexpected behavior, or erroneous states within the contract. Examples include:

The constructor in Keys.sol does not enforce that _protocolFeeDestination is non-zero, nor does it validate that _protocolFeePercent and _subscriberFeePercent fall within acceptable ranges:

constructor(address _protocolFeeDestination, uint256 _protocolFeePercent, uint256 _subscriberFeePercent)
  Ownable(msg.sender)
{
  protocolFeeDestination = _protocolFeeDestination;
  protocolFeePercent = _protocolFeePercent;
  subscriberFeePercent = _subscriberFeePercent;
...

Furthermore, the setters were also missing validations:

function setFeeDestination(address _feeDestination) external onlyOwner {
    protocolFeeDestination = _feeDestination;
}

function setProtocolFeePercent(uint256 _feePercent) external onlyOwner {
    protocolFeePercent = _feePercent;
}

function setSubscriberFeePercent(uint256 _feePercent) external onlyOwner {
    subscriberFeePercent = _feePercent;
}

function setDecayProtocolFeePercent(uint256 _feePercent) external onlyOwner {
    decayProtocolFeePercent = _feePercent;
}

function setDecaySubscriberFeePercent(uint256 _feePercent) external onlyOwner {
    decaySubscriberFeePercent = _feePercent;
}

This list is not exhaustive. It is recommended to conduct a comprehensive review of the codebase to identify and assess other functions that may require additional input validation. Ensuring appropriate checks are in place for critical parameters will enhance the overall reliability, security, and predictability of the contracts.

To mitigate these issues, implement input validation in all constructor functions and other critical functions to ensure that inputs meet expected criteria. This can prevent unexpected behaviors and potential vulnerabilities.

ACKNOWLEDGED: The Fan.Fun team acknowledged the risk of this finding indicating "We are okay with the risks here. We have tests in place to avoid deployment with incorrect parameters".

The project demonstrates inconsistent usage of SPDX license declarations across its files. Specifically, the following licenses are specified in different parts of the codebase:

• // SPDX-License-Identifier: MIT

• // SPDX-License-Identifier: BUSL

  
  

These variations in licensing can lead to potential compatibility and compliance issues, particularly when combining or redistributing code.

• Standardize Licensing: Review the project’s intended licensing model and ensure all files use the same SPDX license declaration to align with that model.

• Segregate Code with Different Licenses: If certain parts of the project require different licenses (e.g., open-source vs proprietary), clearly separate these components into distinct modules or repositories to avoid conflicts.

• Seek Legal Advice: Consult legal experts to verify that the chosen licenses comply with your project’s objectives and that all licensing terms are properly adhered to, minimizing legal exposure.

• Document Licensing Strategy: Provide a clear licensing overview in the project’s documentation or repository (e.g., a LICENSE file or README) to ensure contributors and users understand the licensing terms and requirements.

SOLVED: The Fan.Fun team fixed this finding in commit 6e5ad34 by keeping the interface (IKeys.sol) as MIT to allow anyone to use it without any problem, but using a 1 year BUSL license for Keys.sol. Fan.Fun also added the related license files.