DeFiFoundrySolidity
16,653 OP
View results
Submission Details
Severity: medium
Invalid

Fixed Array Limitations and Missing Route Management in Alchemix Router Contract

Summary

The addRoute function in the StrategyMainnet contract (src/StrategyMainnet.sol) has several design limitations, including fixed array sizes, a lack of bounds checking, and no mechanism for removing or updating routes. These constraints restrict the contract's flexibility and scalability, particularly when the number of routes needs to exceed the fixed limit or when changes to existing routes are required.

Vulnerability Details

Contract: StrategyMainnet.sol
Location: src/StrategyMainnet.sol:56-64
Interface: IStrategyInterface.sol

function addRoute(
address[11] calldata _route,
uint256[5][5] calldata _swapParams, // 5x5 array limits to 5 routes
address[5] calldata _pools // 5-length array confirms max 5 routes
) external onlyManagement {
routes[nRoutes] = _route;
swapParams[nRoutes] = _swapParams;
pools[nRoutes] = _pools;
nRoutes++;
}

Key Issues:

  1. No Bounds Checking on nRoutes:
    In StrategyMainnet.sol, there is no validation to prevent nRoutes from exceeding the maximum size (5) of the arrays routes, swapParams, and pools, risking an out-of-bounds error.

  2. No Mechanism for Route Removal or Updates:
    The contract does not support removing or updating routes in the mappings routes, swapParams, and pools, which could be problematic if routes need to be changed or removed over time.

  3. Fixed Array Sizes:
    The contract uses fixed-size arrays:

    • address[11] for routes

    • uint256[5][5] for swap parameters

    • address[5] for pools
      This design limits the number of routes to 5 and each route to specific dimensions, which may not be scalable for future needs.

  4. No Update Mechanism for Routes:
    Once a route is added to the mappings, there is no functionality to modify it, forcing a full removal and re-insertion process which is currently not possible.

Impact

  • Medium/Low Risk:
    The current limitations in StrategyMainnet.sol do not pose immediate security risks but could impact the scalability and operational flexibility of the contract. If more than 5 routes are needed, or if routes require removal or updates, it may necessitate contract redeployment or significant changes to the protocol, which could disrupt operations.

  • No Direct Security Vulnerability:
    While the vulnerability does not expose the contract to direct attacks, it introduces operational challenges in the Curve Router integration (ICurveRouterNG) that could affect the contract's long-term viability.

  • Potential Operational Constraints:
    As the Alchemix protocol evolves or scales, the lack of route management flexibility may cause operational issues, especially if the system needs more than 5 routes or dynamic updates to the routing configurations for the Curve Router integration.

Tools Used

  • Manual Code Review:
    A thorough review of the addRoute function and its associated logic reveals the design flaws.

  • Solidity Static Analysis:
    Identifying array size limitations and bounds checking issues, which would be flagged in tools like Slither or MythX.

  • Gas Optimization Tools:
    Considerations for optimizing the contract's functionality while maintaining flexibility (e.g., use of dynamic arrays).

Recommendations

  1. Add Bounds Checking: Ensure that nRoutes does not exceed the maximum number of routes (5) by adding a require statement to prevent out-of-bounds writes in StrategyMainnet.sol:

    function addRoute(
    address[11] calldata _route,
    uint256[5][5] calldata _swapParams,
    address[5] calldata _pools
    ) external onlyManagement {
    require(nRoutes < 5, "Max routes reached");
    routes[nRoutes] = _route;
    swapParams[nRoutes] = _swapParams;
    pools[nRoutes] = _pools;
    nRoutes++;
    }
  2. Implement Route Removal Functionality: Add a new function in StrategyMainnet.sol to allow routes to be removed by shifting remaining routes down the arrays to fill the gap:

    function removeRoute(uint256 routeIndex) external onlyManagement {
    require(routeIndex < nRoutes, "Invalid route index");
    for (uint256 i = routeIndex; i < nRoutes - 1; i++) {
    routes[i] = routes[i + 1];
    swapParams[i] = swapParams[i + 1];
    pools[i] = pools[i + 1];
    }
    delete routes[nRoutes - 1];
    delete swapParams[nRoutes - 1];
    delete pools[nRoutes - 1];
    nRoutes--;
    }
  3. Consider Using Dynamic Arrays: Transition to dynamic arrays for better scalability. If this is a future consideration, ensure that appropriate safety checks (e.g., maximum route count) are implemented to prevent excessive gas usage.

  4. Add Route Update Mechanism (Optional): Allow for updates to existing routes instead of requiring their removal and re-insertion. This would reduce gas costs and provide greater flexibility for managing routes.

    function updateRoute(
    uint256 routeIndex,
    address[11] calldata _newRoute,
    uint256[5][5] calldata _newSwapParams,
    address[5] calldata _newPools
    ) external onlyManagement {
    require(routeIndex < nRoutes, "Invalid route index");
    routes[routeIndex] = _newRoute;
    swapParams[routeIndex] = _newSwapParams;
    pools[routeIndex] = _newPools;
    }

By implementing these changes, the contract would gain flexibility, scalability, and operational robustness, ensuring it remains adaptable as the protocol evolves.

Updates

Appeal created

inallhonesty Lead Judge 7 months ago
Submission Judgement Published
Invalidated
Reason: Non-acceptable severity
drlaravel Submitter
7 months ago
inallhonesty Lead Judge
7 months ago
inallhonesty Lead Judge 7 months ago
Submission Judgement Published
Invalidated
Reason: Non-acceptable severity

Support

FAQs

Can't find an answer? Chat with us on Discord, Twitter or Linkedin.