Chaos Labs & BGD Labs Edge Agents Security Review Report

The EdgeAgentHub implementation contract can be directly initialized, allowing an attacker to become its owner, potentially allowing them to execute privileged functions on the implementation contract itself.

contract EdgeAgentHub is EdgeConfigurator, IEdgeAgentHub {
    using EnumerableSet for EnumerableSet.AddressSet;
 
    function initialize(address edgeOwner) external initializer {
        __EdgeConfigurator_init(edgeOwner);
    }

Implement initialization protection using one of these approaches

• Disable initializers in the constructor:

constructor() {
    _disableInitializers();
}

• Add a proxy-only check

function initialize(address edgeOwner) external initializer {
    require(address(_getImplementation()) != address(this), "Direct initialization not allowed");
    __EdgeConfigurator_init(edgeOwner);
}

The EdgeAgentHub exposes a view function check for the automated off-chain component to calculate the ActionData from a list of agent IDs. This action data is later used to execute.

The function uses the global maxBatchSize to limit the amount of inject actions across all agents and the function will silently stop if the maximum has been reached. In such a case, it'll simply return the actions up to the maximum.

This mechanism can be used by a single agent to exploit others. Since the markets are fetched using _getAgentMarkets, which dynamically fetches the markets from the agent address, it can be arbitrarily increased and it can force the maximum batch size to be reached.

For example, if the number of markets is greater than the maximum batch size, the check function will never reach the next agent. The check function only accepts agent IDs as parameters and so it cannot skip to the middle of a market list of an agent where it left off.

For the hypothetical off-chain mechanism there are 2 cases if the agent has more markets than the limit:

• It thinks the agent ID was not fully processed because of the break, so it will include the same agent ID again, blocking it for all other agents.
• It thinks the agent ID was processed because it was included in the parameters, and so it skips over the rest of the markets. This can be abused by an agent, if the break is forced to happen during a victim agent ID, forcing a skip of those markets. If we look at ChainlinkEdgeAgentHub.sol then it would probably be the 2nd case.

function check(
    uint256[] memory agentIds
) public view virtual returns (bool, ActionData[] memory) {
    ActionData[] memory actionData = new ActionData[](agentIds.length);
    uint256 actionCount; // total number of updates across all agents
 
    EdgeHubStorage storage $ = _getStorage();
    uint256 maxBatchSize = $.maxBatchSize;
    uint256 batchSize;
    for (uint256 i = 0; i < agentIds.length; i++) {
        uint256 agentId = agentIds[i];
 
        AgentConfig storage config = $.config[agentId];
        BasicConfig storage basicConfig = config.basicConfig;
 
        if (!basicConfig.isAgentEnabled) continue;
 
        address[] memory markets = _getAgentMarkets(
            config,
            basicConfig.isMarketsFromAgentEnabled,
            basicConfig.agentAddress,
            agentId
        );
 
        if (markets.length == 0) continue;
 
        IRiskOracle riskOracle = IRiskOracle(basicConfig.riskOracle);
        string memory updateType = config.updateType;
 
        address[] memory marketsToUpdate = new address[](markets.length);
        uint256 marketsToUpdateCount;
        for (uint256 j = 0; j < markets.length; j++) {
            // The Risk Oracle is expected to revert if we query a non-existing update.
            // In that case, we simply skip the market
            try riskOracle.getLatestUpdateByParameterAndMarket(updateType, markets[j]) returns (
                IRiskOracle.RiskParameterUpdate memory updateRiskParams
            ) {
                if (_validateBasics(config, basicConfig, agentId, updateRiskParams)) {
                    marketsToUpdate[marketsToUpdateCount++] = updateRiskParams.market;
                    batchSize++;
                }
            } catch {}
 
            // stop collecting data if we reached max batch size, to protect against gas overflow on execution
            if (maxBatchSize != 0 && batchSize == maxBatchSize) break;
        }
 
        if (marketsToUpdateCount != 0) {
            assembly {
                mstore(marketsToUpdate, marketsToUpdateCount)
            }
            actionData[actionCount].agentId = agentId;
            actionData[actionCount].markets = marketsToUpdate;
            actionCount++;
        }
 
        // stop collecting data if we reached max batch size, to protect against gas overflow on execution
        if (maxBatchSize != 0 && batchSize == maxBatchSize) break;
    }
 
    assembly {
        mstore(actionData, actionCount)
    }
 
    return (actionCount != 0, actionData);
}

The check function should allow for picking up where it left off with regard to a list of markets, although this might complicate automation further. An easier approach would be to break earlier, if batchSize + markets.length > maxBatchSize and not fill it all the way to maxBatchSize. That way you only process full agents and the malicious agent can be more easily left out by the offchain component.

Commentary from the client:

We assume that every Hub will be controlled by one entity, i.e., one hub for Aave and another for GMX. So, in this case, it's hard to consider the configuration of agents as a potential exploit.

The function _getAgentMarkets fetches the agent's list of markets and filter it against the restrictedMarkets list by copying everything into another array.

function _getAgentMarkets(
    AgentConfig storage config,
    bool isMarketsFromAgentEnabled,
    address agentAddress,
    uint256 agentId
) internal view returns (address[] memory) {
    // Case we fetch only allowed markets configured on the Hub for this Agent
    if (!isMarketsFromAgentEnabled) {
        return config.allowedMarkets.values();
    }
 
    // Otherwise we fetch all markets from the Agent and apply the configured restricted markets
    address[] memory markets = IBaseAgent(agentAddress).getMarkets(agentId);
 
    uint256 validMarketCount;
    address[] memory validMarkets = new address[](markets.length);
    for (uint256 i = 0; i < markets.length; i++) {
        if (!config.restrictedMarkets.contains(markets[i])) {
            validMarkets[validMarketCount++] = markets[i];
        }
    }
    assembly {
        mstore(validMarkets, validMarketCount)
    }
 
    return validMarkets;
}

We recommend to return the full list early if the restrictedMarkets list is empty and avoid creating an identical array.

For example:

if (config.restrictedMarkets.length() == 0) return markets;

The EdgeConfigurator allows for registering an agent with an arbitrary agent address. It also exposes a configuration function to modify the agent address of an existing agent. Both functions can only be called by the owner.

Nonetheless, there are no safeguards on whether the provided agent address is unique and not already in use by another agent. If a duplicate agent address were to be submitted (e.g. mistakenly or because of automation), it could result in one agent pivoting into another victim agent, as the agent trusts the call of the EdgeAgentHub. By only allowing the address to be used by one agent at a time, this would be prevented.

Moreover, the current Agent implementations do not validate the parameter agentId or updateType in _processUpdate, so it would not know if another agent with another ID has forced a call into it.

function setAgentAddress(uint256 agentId, address agentAddress) public onlyOwner {
    _getStorage().config[agentId].basicConfig.agentAddress = agentAddress;
    emit AgentAddressSet(agentId, agentAddress);
}

We recommend to keep a mapping of agentAddress => bool and verify whether the address has been used already when registering or modifying an agent's address.

Commentary from the client:

It is the design decision, we imagine cases when one agent contract can be "reused" by a different agentId, and may support multiple updateTypes.