Skip to content

getSuggestedFees: response schema defined but never validated; negative amounts accepted #265

@Nexory

Description

@Nexory

Summary

getSuggestedFees fetches the /suggested-fees API endpoint and passes the raw response directly to parseSuggestedFees without ever running it through the Zod schema defined for exactly this purpose. As a secondary issue, the shared bigNumberString validator in validators.ts permits negative values via its /^-?\d+$/ regex, so a malformed or adversarial API response containing a negative outputAmount or fee total would silently pass type-checking and reach downstream callers — including viem ABI encoding for SpokePool deposits.

What I observed

packages/sdk/src/actions/getSuggestedFees.ts L174–186:

export async function getSuggestedFees({ apiUrl = MAINNET_API_URL, logger, ...params }) {
  const data = await fetchAcrossApi<SuggestedFeesApiResponse>(
    `${apiUrl}/suggested-fees`,
    params,
    logger,
  );
  return parseSuggestedFees(data);  // no schema validation before this point
}

The schema suggestedFeesResponseJsonSchema is defined in packages/sdk/src/api/suggested-fees.ts and exported, but is never called (no .parse() or .safeParse()). The fetchAcrossApi utility in utils/fetch.ts is a generic HTTP fetcher — its type parameter <SuggestedFeesApiResponse> is a TypeScript cast only; no runtime validation occurs there.

packages/sdk/src/api/validators.ts L20–22:

export const bigNumberString = z.string().regex(/^-?\d+$/, {
  message: "Invalid BigNumber string format",
});

bigNumberString is used for outputAmount, all fee .total fields, and all limits.* fields in the suggested-fees schema. The leading -? means negative strings like "-1" pass validation and are then converted via BigInt("-1") in parseSuggestedFees.

By contrast, getSwapQuote (same package, actions/getSwapQuote.ts L102–113) explicitly calls swapApprovalResponseSchema.safeParse(data) and throws a descriptive error on mismatch — demonstrating the intended pattern is already established in the codebase.

Impact

  • A malformed or adversarial API response (e.g. outputAmount: "-1") reaches callers undetected. When the caller forwards these values to a SpokePool deposit transaction, viem ABI encoding may throw an opaque error or, depending on the uint type, silently wrap to a very large integer.
  • Even for non-adversarial scenarios (API schema drift, staging vs production mismatches), developers receive no actionable error message — the failure surfaces deep in transaction construction rather than at the fetch boundary.
  • Correctness gap: the schema was clearly authored as a validation contract but is never enforced, so its presence gives false assurance to readers of the code.

Suggested fix

Option A (minimal — mirrors getSwapQuote pattern): validate in getSuggestedFees before parsing:

import { suggestedFeesResponseJsonSchema } from "../api/suggested-fees.js";

export async function getSuggestedFees({ apiUrl = MAINNET_API_URL, logger, ...params }) {
  const raw = await fetchAcrossApi<unknown>(
    `${apiUrl}/suggested-fees`,
    params,
    logger,
  );
  const validated = suggestedFeesResponseJsonSchema.safeParse(raw);
  if (!validated.success) {
    throw new Error(`Invalid suggested-fees response: ${validated.error.message}`);
  }
  return parseSuggestedFees(validated.data);
}

Option B (defense in depth — remove -? from bigNumberString): amounts returned by this API are always non-negative; allowing negative strings is a latent footgun regardless of whether the schema is called:

// validators.ts L20
export const bigNumberString = z.string().regex(/^\d+$/, {
  message: "Invalid BigNumber string format",
});

Note: if negative bigints are intentionally needed elsewhere, a separate signedBigNumberString validator could be introduced for those use cases.

Recommended: apply both fixes for defense in depth.

Notes

bigNumberString is shared across multiple response schemas in this package (swap-approval, etc.). Changing its regex would affect all callers — worth a quick audit of whether any field legitimately needs negative values before merging Option B.

Metadata

Metadata

Assignees

No one assigned

    Labels

    No labels
    No labels

    Type

    No type
    No fields configured for issues without a type.

    Projects

    No projects

    Milestone

    No milestone

    Relationships

    None yet

    Development

    No branches or pull requests

    Issue actions