Agent Exchange Pro League
Findings & Analysis Report

2024-06-18

Table of contents

Overview

About C4

Code4rena (C4) is an open organization consisting of security researchers, auditors, developers, and individuals with domain expertise in smart contracts.

A C4 Pro League Audit is an event where elite tier Code4rena contributors, commonly referred to as wardens, reviews, audits and analyzes smart contract logic in exchange for a bounty provided by sponsoring projects.

During the Pro League audit outlined in this document, C4 conducted an analysis of the Agent Exchange smart contract system written in Solidity. The audit took place between May 28 - June 14, 2024.

Wardens

2 Wardens contributed to Agent Exchange:

  1. MiloTruck
  2. HollaDieWaldfee

Final report assembled by bytes032 and thebrittfactor.

Summary

The C4 Pro League analysis included 6 findings with a risk rating of LOW severity or non-critical and 4 Informational findings.

Scope

The source code was delivered to Code4rena in a private Git repository.

Severity Criteria

C4 assesses the severity of disclosed vulnerabilities based on three primary risk categories: high, medium, and low/non-critical.

High-level considerations for vulnerabilities span the following key areas when conducting assessments:

  • Malicious Input Handling
  • Escalation of privileges
  • Arithmetic
  • Gas use

For more information regarding the severity criteria referenced throughout the submission review process, please refer to the documentation provided on the C4 website, specifically our section on Severity Categorization.

Low Risk Findings (6)

[L-01] Missing upper bound check for fees in AgentExchangeV1.initialize()

Lines of Code

  • AgentExchangeV1.sol#L248-L254
  • AgentExchangeV1.sol#L67

Description

In AgentExchangeV1.sol, when fees are set by the owner using setFees(), they are checked to be less than or equal to 1e8:

function setFees(uint256 _fees) external onlyOwner {
    if (_fees > 1e8) {
        revert InvalidFees();
    }
    fees = _fees;

However, this check does not exist in initialize() even though fees is also set. As a result, when the AgentExchangeV1 contract is first initialized, fees can be set to any arbitrary value greater than 1e8.

Recommendation

In AgentExchangeV1.initialize(), check that _fees is not greater than 1e8:

  function initialize(...)
      public
      initializer
  {
+     if (_fees > 1e8) revert InvalidFees();
      _initializeOwner(_owner);
      pool = IAgentPool(_pool);
      feeOracle = _feeOracle;
      fees = _fees;

Agent Exchange

Fixed.

Code4rena Pro League

The recommendation has been implemented.

[L-02] AgentPool.rescueFunds() sends funds to the owner or treasury based on _token

Lines of Code

AgentPool.sol#L126-L131

Description

In AgentPool.rescueFunds(), ETH is rescued to msg.sender whereas other tokens are rescued to treasury:

if (_token == ETH) {
    (bool sent,) = msg.sender.call{value: _amount}("");
    require(sent, "Failed to send Ether");
} else {
    IERC20(_token).safeTransfer(treasury, _amount);
}

This seems inconsistent - tokens should always be rescued to one address, regardless of when it is ETH or other ERC20 tokens.

Recommendation

Amend rescueFunds() to either ETH to the treasury, or send all other tokens to msg.sender.

Agent Exchange

Fixed.

Code4rena Pro League

The recommendation has been implemented. All rescued funds are sent to the treasury.

[L-03] Rebasing tokens are not supported

Lines of Code

AgentPool.sol#L59-L63

Description

Under “ERC20 token behaviors in scope” in the scoping form, tokens with ”balance changes outside of transfers” (i.e., rebasing tokens) are marked as supported.

However, the current design of AgentPool is not able to support rebasing tokens; especially ones where balances can decrease. When users deposit tokens through AgentPool.deposit(), the amount deposited is recorded in a balances mapping:

uint256 balanceBefore = IERC20(_token).balanceOf(address(this));
IERC20(_token).safeTransferFrom(msg.sender, address(this), _amount);
uint256 balanceAfter = IERC20(_token).balanceOf(address(this));

balances[_token][_account] += balanceAfter - balanceBefore;

These token balances are then used to trade NFTs in AgentExchangeV1.

However, if _token is a rebasing token, over time, the balances mapping will not accurately reflect the amount of tokens belonging to a user. For example, if _token rebases down, the actual token balance held in the contract will be lower than the amount stored in the balances mapping.

This could cause unexpected behavior in the protocol; for example, AgentPool.withdraw() could revert even when the user’s balances mapping is more than _amount.

Note that on Blast L2, WETH and USDB are rebasing tokens by default. However, they can be used in the protocol as both tokens only rebase up (i.e., the balance held by an address never decreases). They can also be configured to not rebase by calling IERC20Rebasing.configure() for both tokens in AgentPool.initialize().

Recommendation

Consider adding a token whitelist to AgentPool, which prevents rebasing tokens and other incompatible tokens from being used.

Alternatively, document that AgentPool is not compatible with rebasing tokens.

Agent Exchange

Fixed.

Code4rena Pro League

The issue has been fixed by implementing a whitelist. Tokens that are not whitelisted by the owner cannot be deposited into AgentPool and listings with non-whitelisted tokens cannot be created.

[L-04] AgentPool.deposit() is incompatible with ERC-777 tokens

Lines of Code

AgentPool.sol#L59-L63

Description

Under “ERC20 token behaviors in scope” in the scoping form, “ERC777 used by the protocol” is marked as “Any”, which means ERC777 tokens are supported..

When users deposit tokens through AgentPool.deposit(), the amount added to their balance is calculated as the difference between the balance before and after the transfer:

uint256 balanceBefore = IERC20(_token).balanceOf(address(this));
IERC20(_token).safeTransferFrom(msg.sender, address(this), _amount);
uint256 balanceAfter = IERC20(_token).balanceOf(address(this));

balances[_token][_account] += balanceAfter - balanceBefore;

However, when _token is an ERC777 token, the caller can use the tokensToSend hook to re-enter deposit() to deposit more tokens during the transfer. For example:

  • Call deposit() with 100 tokens.

    • Assume balanceBefore = 0.
    • _token.transferFrom() is called, which calls the tokensToSend hook and gives execution control to msg.sender:
    • The caller re-enters deposit() and deposits another 100 tokens. This adds 100 tokens to the caller’s balance.
    • _token.transferFrom() transfers 100 tokens to the contract.
    • balanceAfter = 200, which adds 200 tokens to the caller’s balance.
  • The caller now has a balance of 300 tokens, even though he only deposited 200 tokens.

As seen from above, balanceAfter - balanceBefore in the initial deposit() call will also include the balance of the re-entered deposit() call, causing a double-counting of the user’s deposit.

Recommendation

Consider reverting the change in Agent.deposit() to not support fee-on-transfer tokens:

balances[_token][_account] += _amount;
IERC20(_token).safeTransferFrom(msg.sender, address(this), _amount);

Otherwise, document that ERC777 tokens are not supported.

Agent Exchange

Fixed.

Code4rena Pro League

The issue has been fixed by implementing a whitelist. Tokens that are not whitelisted by the owner cannot be deposited into AgentPool and listings with non-whitelisted tokens cannot be created.

[L-05] ETH cannot be deposited for another user

Lines of Code

AgentPool.sol#L51-L66

Description

ETH cannot be deposited via AgentPool.deposit() since it reverts when _token=ETH. It can only be deposited by triggering the receive() function.

receive() external payable {
    balances[ETH][msg.sender] += msg.value;
    emit Received(ETH, msg.sender, msg.value);
}

Since receive() always makes the deposit for msg.sender, it is not possible to deposit ETH for another user. This makes ETH different from other tokens, which can lead to problems when components integrate with AgentPool, expecting that ETH can be deposited for other users.

Recommendation

Consider changing AgentPool.deposit() to allow ETH deposits.

-    function deposit(address _token, address _account, uint256 _amount) public {
-        if (_token == ETH) {
-            revert InvalidToken();
+    function deposit(address _token, address _account, uint256 _amount) public payable {
+        if (_token != ETH) {
+            require(msg.value == 0);
         }
         if (_amount == 0) {
             revert InvalidAmount();
         }

-        uint256 balanceBefore = IERC20(_token).balanceOf(address(this));
-        IERC20(_token).safeTransferFrom(msg.sender, address(this), _amount);
-        uint256 balanceAfter = IERC20(_token).balanceOf(address(this));
+        if (token != ETH) {
+            uint256 balanceBefore = IERC20(_token).balanceOf(address(this));
+            IERC20(_token).safeTransferFrom(msg.sender, address(this), _amount);
+            uint256 balanceAfter = IERC20(_token).balanceOf(address(this));

-        balances[_token][_account] += balanceAfter - balanceBefore;
+            balances[_token][_account] += balanceAfter - balanceBefore;
+        } else {
+            require(msg.value == _amount);
+            balances[_token][_account] += _amount;
+        }

         emit Received(_token, _account, _amount);
     }

Applying this change, it is also possible to remove the receive() function.

Agent Exchange

Fixed.

Code4rena Pro League

The recommendation has been implemented. Also, the balanceAfter - balanceBefore difference is no longer checked. Instead it is assumed that no fee-on-transfer tokens will be used and tokens are now whitelisted by the owner.

[L-06] Check length of feeOracle signature

Lines of Code

AgentExchangeV1.sol#L280-L289

Description

For signatures that are provided by bidders, it is checked that their length is equal to 65 bytes. This is to prevent the use of malleable signatures. However, the same check is missing from the _verifyOracleSignature() function which checks the signatures of the feeOracle.

Recommendation

It is recommended to check that the feeOracle signature length is equal to 65 bytes.

Agent Exchange

Fixed.

Code4rena Pro League

The recommendation has been implemented.

Informational Findings (4)

[I-01] Initializable is used without calling _disableInitializers() in the constructor

Lines of Code

  • AgentPool.sol
  • AgentExchangeV1.sol

Description

Solady’s Initializable.sol recommends calling _disableInitializers() in the constructor of implementation contracts:

/// @dev Locks any future initializations by setting the initialized version to `2**64 - 1`.
///
/// Calling this in the constructor will prevent the contract from being initialized
/// or reinitialized. It is recommended to use this to lock implementation contracts
/// that are designed to be called through proxies.
///
/// Emits an {Initialized} event the first time it is successfully called.
function _disableInitializers() internal virtual {

This prevents users from initializing the implementation contract, which could affect the proxy contract under certain conditions (eg. the implementation contract can be self-destructed).

In this protocol, an attacker initializing the implementation contracts of AgentPool or AgentExchangeV1 will have no impact on the proxy contracts. Nevertheless, it is best practice to call _disableInitializers() to prevent attackers from doing so.

Recommendation

In both AgentPool and AgentExchangeV1, add a constructor that calls _disableInitializers():

 constructor() {
    _disableInitializers();
 }

Agent Exchange

Acknowledged. It doesn’t affect proxied contracts, also our implementation will not be self destructable so we can skip this.

Code4rena Pro League

The finding does not have a security impact. It is okay to acknowledge it.

[I-02] Natspec for AgentExchangeV1.listItem() has missing parameters

Lines of Code

AgentExchangeV1.sol#L71-L77

Description/Recommendation

The natspec for AgentExchangeV1.listItem() is missing the token and expiry parameter:

  /*
   * @notice Method for listing NFT
   * @param nftAddress Address of NFT contract
   * @param tokenId Token ID of NFT
+  * @param token sale token for each item
   * @param price sale price for each item
+  * @param expiry expiry timestamp for the listing
   */
  function listItem(address nftAddress, uint256 tokenId, address token, uint256 price, uint256 expiry)

Agent Exchange

Fixed.

Code4rena Pro League

The recommendation has been implemented.

[I-03] InsufficientBalance error is unused

Lines of Code

IAgentExchangeV1Utils.sol#L31

Description/Recommendation

Consider removing the InsufficientBalance error from IAgentExchangeV1Utils since it is never used anywhere.

Agent Exchange

Fixed.

Code4rena Pro League

The recommendation has been implemented.

[I-04] Oracle signature does not include bid.amount and price based discounts cannot be implemented

Lines of Code

AgentExchangeV1.sol#L186-L219

Description

In AgentExchangeV1.takeBid(), it is possible that item.amount != bid.amount. This means buyer and seller can agree on a price that is different from the price of the listing.

However, the oracle only signs the price in item.amount, not bid.amount, so the oracle cannot consider the actual price of the sale when calculating its discount. It has been determined by the client that this is not a concern since the discount will not depend on the price of the sale.

Recommendation

To allow for discount models that rely on the price of the sale, it is recommended to include bid.amount in the data that the feeOracle needs to sign.

Agent Exchange

Fixed.

Code4rena Pro League

Fixed as recommended. For bids, bid.amount is part of the oracle signature. For asks, item.amount is part of the oracle signature.

Disclosures

C4 is an open organization governed by participants in the community.

C4 audits incentivize the discovery of exploits, vulnerabilities, and bugs in smart contracts. Security researchers are rewarded at an increasing rate for finding higher-risk issues. Audit submissions are judged by a knowledgeable security researcher and solidity developer and disclosed to sponsoring developers. C4 does not conduct formal verification regarding the provided code but instead provides final verification.

C4 does not provide any guarantee or warranty regarding the security of this project. All smart contract software should be used at the sole risk and responsibility of users.