Smart Contracts - Fraction AI

The reportStake() and validateStake() functions in the Space contract use a signature verification mechanism that doesn't bind the signature to the caller (msg.sender). The signature only verifies the session parameters (sessionId, participantId, amount) but doesn't include the intended user's address in the signed message.

function reportStake(
	uint256 sessionId,
	uint256 participantId,
	uint256 amount,
	bytes calldata signature
) external nonReentrant {
	Session memory contractSession = sessionMap[sessionId];
	require(contractSession.state == State.IN_PROGRESS, "Invalid session");
	require(uint32(block.timestamp) < contractSession.startedAt + sessionCooldown, "Too late");
	require(_verifyReportStake(sessionId, participantId, amount, signature), "Bad Sig");
	AgentContest storage contest = agentReports[sessionId][participantId];
	_contestStake(amount, contest);
	emit ReportStaked(msg.sender, sessionId, participantId, amount);
}

function validateStake(
	uint256 sessionId,
	uint256 participantId,
	uint256 amount,
	bytes calldata signature
) external nonReentrant {
	Session memory contractSession = sessionMap[sessionId];
	require(contractSession.state == State.IN_PROGRESS, "Invalid session");
	require(uint32(block.timestamp) < contractSession.startedAt + sessionCooldown, "Too late");
	require(_verifyValidateStake(sessionId, participantId, amount, signature), "Bad Sig");
	AgentContest storage contest = agentValidations[sessionId][participantId];
	_contestStake(amount, contest);
	emit ValidateStaked(msg.sender, sessionId, participantId, amount);
}

function _verifyReportStake(
	uint256 sessionId,
	uint256 participantId,
	uint256 amount,
	bytes calldata signature
) internal view returns (bool) {
	bytes32 hash = keccak256(abi.encode("REPORT_STAKE", address(this), sessionId, participantId, amount));
	return isProtocol[ECDSA.recover(MessageHashUtils.toEthSignedMessageHash(hash), signature)];
}

function _verifyValidateStake(
	uint256 sessionId,
	uint256 participantId,
	uint256 amount,
	bytes calldata signature
) internal view returns (bool) {
	bytes32 hash = keccak256(abi.encode("VALIDATE_STAKE", address(this), sessionId, participantId, amount));
	return isProtocol[ECDSA.recover(MessageHashUtils.toEthSignedMessageHash(hash), signature)];
}

This could allow for an attacker to frontrun legitimate users' transactions and become the recorded "contestant" for reports and validations.

function _contestStake(uint256 amount, AgentContest storage contest) internal {
	require(contest.contestant == address(0), "Already exists");
	agentController.updateUserContestStake(msg.sender, token, amount, false);
	contest.contestant = msg.sender;
	contest.stakeAmount = amount;
	contest.createdAt = block.timestamp;
}

This can cause:

1. Direct Financial Gain: When validations are approved, the attacker (as the contestant) receives slashed funds directly from nodes through the nodeController.slashStake() function, in the validatePass() function. This creates a financial incentive for the attack.

   nodeController.slashStake(contest.contestant, nodeIds[i], taskIds[i]);

   
   
   

2. Denial of Service: Legitimate users who prepared signatures will have their transactions fail with "Already exists" errors, preventing them from participating in the protocol's security mechanisms.

Include the intended user's address in the signed message hash to bind signatures to specific users, and modify the verification functions to check that msg.sender matches the address included in the signed message.

Additionally, consider adding a nonce or timestamp mechanism to the signatures to prevent signature reuse.

SOLVED: The Fraction AI team solved this finding in commit 72a4297 by following the mentioned recommendation.

The AgentController contract may be vulnerable to cross-chain signature replay attacks under specific deployment conditions. The _verifyKey() function, used during agent creation, incorporates the contract address but omits the chain ID in the signature verification process, creating a potential security risk.

function _verifyKey(uint256 key, bytes calldata signature) internal view returns (bool) {
    bytes32 hash = keccak256(abi.encode("AGENT_CREATE", address(this), key));
    return isProtocol[ECDSA.recover(MessageHashUtils.toEthSignedMessageHash(hash), signature)];
}

If the following conditions are met, signatures could be replayed across chains:

1. The AgentController contract is deployed at identical addresses on multiple chains (possible with deterministic deployment methods like CREATE2).

2. The same signer is authorized in the isProtocol mapping on both chains.

Under these conditions, a valid signature for agent creation on one chain could be reused on another chain to create the same agent without the signer's knowledge or consent, potentially allowing unauthorized agent creation.

Take the following scenario:

1. A protocol operator (with address X) is authorized in the isProtocol mapping on both Ethereum and Base.

2. The protocol operator signs a message to create Agent A for User B on Ethereum.

3. An attacker captures this signature and replays it on Base.

4. The signature verification passes on Base because:

• The contract addresses are identical.

  
  

• The signer (X) is authorized in the isProtocol mapping on Base.

  
  

• The signature correctly corresponds to the agent key.

Include the chain ID in the signature message to make signatures chain-specific.

SOLVED: The Fraction AI team solved this finding in commit 72a4297 by following the mentioned recommendation.

The AgentNFT contract enforces a call to the transferAgentOwnership() function of the AgentController contract when transferring an NFT.

function _update(address to, uint256 tokenId, address auth) internal override returns (address) {
    address from = super._update(to, tokenId, auth);
    // Transfer only
    if (from != address(0)) {
        agentController.transferAgentOwnership(tokenId, from, to);
    }
    return from;
}

However, the contract does not handle the case where the recipient is a contract that does not implement the onERC721Received() function.

This could lead to tokens being lost if a token owner executes the transfer of the NFT via the transferFrom() function instead of the safeTransferFrom() function, and the recipient is a contract that can't handle NFT tokens.

Consider enforcing the use of safeTransferFrom() instead of transferFrom() to ensure that the contract recipients can handle the transfers correctly.

This could be done by overriding the transferFrom() function to make a call to the ERC721Utils.checkOnERC721Received() function and verify that the recipient contract can handle the transfer correctly.

RISK ACCEPTED: The Fraction AI team made a business decision to accept the risk of this finding and not alter the contracts, stating that:

NFTs in the current implementation will only be used for agent ownership. The users are responsible for handling their NFTs. Also, we will be creating smart wallets for all the users using our protocol interface, and will be managing all contract interactions on their behalf. Since smart wallets will be generated using third-party services, we do not wish to be limited in case it doesn't implement the ERC721 receiving functionality. So safeTransferFrom will be used in our interface, but otherwise the users are responsible for managing their NFTs.

In the AgentController contract, there's an inconsistency in how ETH deposits and withdrawals are handled. When users deposit ETH using the addBalance() function, the contract converts address(0) to the WETH address and records the balance under address(weth). However, when users try to withdraw using withdrawBalance(), the function does not automatically handle the case where token is set to address(0) (representing ETH).

Users who deposit ETH and later try to withdraw by passing address(0) as the token will find their withdrawal requests failing. This happens because:

1. Their ETH balance is stored under address(weth) mapping.

   
   
   

2. The withdrawal attempts to read from userBalances[msg.sender][address(0)] which will be zero.

   
   
   

3. The transaction will revert with "Balance too low" or "Amount too low".

This inconsistency creates a confusing user experience and could lead to users being unable to access their funds through the normal interface.

Modify the withdrawBalance function to handle ETH withdrawals consistently, similarly to the way they are handled in the addBalance() function.

SOLVED: The Fraction AI team solved this finding in commit 72a4297 by following the mentioned recommendation.

The AgentController, Space and NodeController contracts in scope follow the upgradeable pattern, inheriting from the Initializable module from OpenZeppelin. In order to prevent leaving the contracts uninitialized, OpenZeppelin's documentation recommends adding the _disableInitializers function in the constructor to automatically lock the contracts when they are deployed. However, these contracts are missing this function call.

This omission can lead to potential security vulnerabilities, as an uninitialized implementation contract can be taken over by an attacker, which may impact the proxy.

Add a constructor and call the _disableinitializers() method within the contracts to prevent the implementation from being initialized. For example:

/// @custom:oz-upgrades-unsafe-allow constructor
constructor() {
  _disableInitializers();
}

For more reference, see OpenZeppelin's recommendation from their documentation, or this discussion from their forum.

SOLVED: The Fraction AI team solved this finding in commit 72a4297 by following the mentioned recommendation.

All contracts in scope inherit the Ownable or OwnableUpgradeable contract implementation from OpenZeppelin's library and is used to restrict access to certain functions to the contract owner. The Ownable pattern allows the contract owner to transfer ownership to another address using the transferOwnership() function. However, the transferOwnership() function does not include a two-step process to transfer ownership.

Regarding this, it is crucial that the address to which ownership is transferred is verified to be active and willing to assume ownership responsibilities. Otherwise, the contract could be locked in a situation where it is no longer possible to make administrative changes to it.

Additionally, the renounceOwnership() function allows renouncing to the owner permission. Renouncing ownership before transferring it would result in the contract having no owner, eliminating the ability to call privileged functions.

Consider using OpenZeppelin's Ownable2Step and Ownable2StepUpgradeable contracts over the Ownable and OwnableUpgradeable implementations. Ownable2Step provides a two-step ownership transfer process, which adds an extra layer of security to prevent accidental ownership transfers.

Additionally, it is recommended that the owner cannot call the renounceOwnership() function to avoid losing ownership of the contract.

RISK ACCEPTED: The Fraction AI team made a business decision to acknowledge this finding and not alter the contracts, stating that:

The owner will be a multisig requiring multiple signatures, so all owner functions will be thoroughly checked before execution.

The Space contract's startSessions() and finishSessions() functions allow duplicate agent keys in the session data arrays, which may leads to users being charged multiple times for the same agent and enables manipulation of reward distributions.

When the Space contract calls AgentController.takeSessionsEntryFees() during startSessions, the _deductEntryFee() function processes each agent key separately without checking for duplicates:

function _deductEntryFee(
    address token,
    uint256 entryFee,
    IAgentController.StartSessionData calldata session
) internal {
    uint256 numAgents = session.agentKeys.length;
    for (uint256 i=0; i < numAgents;) {
        uint256 agentKey = session.agentKeys[i];
        address agentUser = agentOwners[agentKey];
        uint256 userBalance = userBalances[agentUser][token];
        require(userBalance >= entryFee, "Bal low");
        userBalances[agentUser][token] = userBalance - entryFee;
        unchecked {i++;}
    }
}

Similarly, during finishSessions with AgentController.updateBatchAgentRewards(), the _updateAgentReward() function also processes duplicate agent keys separately:

function _updateAgentReward(
    address token,
    uint256 entryFee,
    uint256 treasuryFee,
    IAgentController.FinishSessionData calldata session
) internal returns (uint256 treasuryAmount) {
    uint256 numAgents = session.agentKeys.length;
    uint256 sessionFees = entryFee * numAgents;
    treasuryAmount = sessionFees * treasuryFee / 100_00;
    uint256 sessionRewards = sessionFees - treasuryAmount;
    uint256 totalRewardPercent;
    for (uint256 i=0; i < numAgents;) {
        address agentUser = agentOwners[session.agentKeys[i]];
        uint256 rewardPercent = session.rewardPercents[i];
        if (rewardPercent > 0) {
            uint256 rewardAmount = sessionRewards * rewardPercent / 100_00;
            userBalances[agentUser][token] += rewardAmount;
            totalRewardPercent += rewardPercent;
        }
        unchecked {i++;}
    }
    require(totalRewardPercent == 100_00, "Reward != 100%");
}

This could lead to:

• Accidental overcharging of users if protocol services include duplicate agent keys.

• Unintended reward distribution if duplicate keys have different reward percentage.

Implement input validation to detect duplicated within the agent keys and ensure each agent key is only processed once per session.

ACKNOWLEDGED: The Fraction AI team made a business decision to acknowledge this finding and not alter the contracts, stating that:

Even if duplicate agent keys are handled, it will not resolve the issue of having different agent keys than expected. The bigger issue is to check whether the agent keys are correct, which cannot be done completely on-chain. It has been solved by combining on-chain event emits by nodes and checking of ipfs CIDs, which allows post-verification of sessions. The protocol will always try to ensure that no malicious activity is present.

In the AgentNFT contract's mint() function, token URIs are set during the minting process without any validation. The function accepts any string as a URI parameter and assigns it to the token without checking if it's empty or if it's already being used by another token.

The lack of validation could lead to several issues:

1. Empty URIs: Tokens could be minted with empty URIs, leading to metadata resolution failures in applications that interact with the NFTs.

   
   
   

2. Duplicate Metadata: Multiple tokens could reference identical metadata, causing confusion for users who might expect unique representations for different agent NFTs

   
   

This creates inconsistency in the NFT collection and could hinder proper functionality in applications that rely on well-formed and unique metadata for each token.

Additionally, once a token URI is set during minting, there's no mechanism to update it. This is not necessarily an issue if immutable metadata is intended, but could be a limitation if metadata updates are needed later.

Implement validation for token URIs at minting time:

1. Add checks to ensure URIs are not empty.

2. Consider adding an optional uniqueness check if tokens should have distinct metadata.

ACKNOWLEDGED: The Fraction AI team made a business decision to acknowledge this finding and not alter the contracts, stating that:

Agent NFTs will mainly be used to handle agent ownership by the protocol. We will not use any metadata in our core functionality.

Throughout the contracts in scope, there are several instances where administrative functions change contract state by modifying core state variables without them being reflected in event emissions.

The absence of events may hamper effective state tracking in off-chain monitoring systems.

Instances of this issue can be found in:

• AgentController.setProtocol()

• AgentController.setTreasury()

• AgentController.setAgentNFT()

• AgentController.setAuthorizedSpace()

• AgentController.takeSessionsEntryFees()

• AgentController.updateBatchAgentRewards()

• AgentController.updateContestFailTreasury()

• AgentController.withdrawTreasury()

• AgentNFT.setAgentController()

• NodeController.setWhitelistedNode()

• NodeController.setAuthorizedSpace()

• Space.setProtocol()

• Space.setAgentController()

• Space.setNodeController()

Emit events for all state changes that occur as a result of administrative functions to facilitate off-chain monitoring of the system.

ACKNOWLEDGED: The Fraction AI team made a business decision to acknowledge this finding and not alter the contracts, stating that:

Events are present in all relevant functions. The remaining functions are administrative functions which are mostly executed once on contract creation and are irrelevant for the normal user.

Throughout the codebase, there are several instances where input values are assigned without proper validation. For example, ensuring that an input address is not the zero address or that an integer falls within a valid range.

Failing to validate inputs before assigning them to state variables can lead to unexpected system behavior or even complete failure.

Instances of this issue include:

• The agentController address in the setAgentController() function of the AgentNFT contract is not checked against the zero address.

  
  

• The _token address in the initialize() function of the NodeController contract is not checked against the zero address.

  
  

• The minStakeAmount value in the initialize() and the setMinStakeAmount() functions of the NodeController contract is not checked to fall within a valid threshold (greater than zero).

  
  

• The nodeHash parameter in the createNode() and updateNode() functions of the NodeController contract is not checked to not be empty.

  
  

• The _contestCooldown and _sessionCooldown parameters in the setCooldowns() function of the Space contract is not checked to fall within a valid threshold (greater than zero).

Add proper validation to ensure that the input values are within expected ranges and that addresses are not the zero address. This will help prevent unexpected behavior and improve the overall robustness of the code.

PARTIALLY SOLVED: The FractionAI team partially solved this finding in commit 72a4297 by following the mentioned recommendation, and stating that:

Input validation added for public functions. For owner functions, the multisig structure will ensure thorough checks before execution.

The takeSessionsEntryFees() function processes multiple sessions in a batch operation, where each session may contain multiple agents. If any single agent has insufficient balance to cover the entry fee, the entire transaction reverts due to the check in the _deductEntryFee() function.

This vulnerability could lead to a denial-of-service condition for the Space contract's startSessions() functionality. When processing large batches of agents, if just one agent has insufficient funds, the entire transaction will revert, preventing all other legitimate agents from participating.

This inefficiency could:

1. Block protocol operation during peak usage times.

2. Force unnecessary gas expenditure on failed transactions.

3. Create a poor user experience for participants with sufficient funds.

4. Enable a malicious actor to deliberately halt sessions by participating with insufficient funds.

Implement a more resilient approach that handles individual agent failures without affecting the entire batch. This could be done by either:

1. Creating a filtering mechanism that processes only agents with sufficient balances while tracking and returning information about which agents were processed successfully.

   
   
   

2. Implementing a non-blocking collection approach where agents with insufficient funds are skipped and reported in an event.

ACKNOWLEDGED: The Fraction AI team made a business decision to acknowledge this finding and not alter the contracts, stating that:

We will always check user balances before creating sessions in our system. We do not wish to add any extra on-chain checks to keep gas costs minimal.

All contracts in scope currently uses floating pragma versions ^0.8.27 which means that the code can be compiled by any compiler version that is greater than or equal to 0.8.27, and less than 0.9.0.

However, it is recommended that contracts should be deployed with the same compiler version and flags used during development and testing. Locking the pragma helps to ensure that contracts do not accidentally get deployed using another pragma. For example, an outdated pragma version might introduce bugs that affect the contract system negatively.

Additionally, from Solidity versions 0.8.20 through 0.8.24, the default target EVM version is set to Shanghai, which results in the generation of bytecode that includes PUSH0 opcodes. Starting with version 0.8.25, the default EVM version shifts to Cancun, introducing new opcodes for transient storage, TSTORE and TLOAD.

In this aspect, it is crucial to select the appropriate EVM version when it's intended to deploy the contracts on networks other than the Ethereum mainnet, which may not support these opcodes. Failure to do so could lead to unsuccessful contract deployments or transaction execution issues.

Lock the pragma version to the same version used during development and testing and make sure to specify the target EVM version when using Solidity versions from 0.8.20 and above if deploying to chains that may not support newly introduced opcodes.

Additionally, it is crucial to stay informed about the opcode support of different chains to ensure smooth deployment and compatibility.

SOLVED: The Fraction AI team solved this finding in commit 72a4297 by following the mentioned recommendation.

Throughout the files in scope, there are several instances where revert strings are used over custom errors.

In Solidity development, replacing hard-coded revert message strings with the Error() syntax is an optimization strategy that can significantly reduce gas costs. Hard-coded strings, stored on the blockchain, increase the size and cost of deploying and executing contracts.

The Error() syntax allows for the definition of reusable, parameterized custom errors, leading to a more efficient use of storage and reduced gas consumption. This approach not only optimizes gas usage during deployment and interaction with the contract but also enhances code maintainability and readability by providing clearer, context-specific error information.

Consider replacing all revert strings with custom errors. For example:

error ConditionNotMet();

if (!condition) revert ConditionNotMet();

or starting from Solidity 0.8.27 :

require(condition, ConditionNotMet());

For more references, see here and here.

ACKNOWLEDGED: The Fraction AI team made a business decision to acknowledge this finding and not alter the contracts, stating that:

No security issues here. We do not mind the increased (one-time) deployment costs. The cost on running functions is negligible. We will keep this in mind in our future versions.

The project contains several unnamed mappings despite using a Solidity version that supports named mappings.

Named mappings improve code readability and self-documentation by explicitly stating their purpose.

Consider refactoring the mappings to use named arguments, which will enhance code readability and make the purpose of each mapping more explicit. For example:

mapping(address myAddress => bool myBool) public myMapping;

SOLVED: The Fraction AI team solved this finding in commit 72a4297 by following the mentioned recommendation.