From 32d0a67c2130f5f893bd824ccfb05007418cf191 Mon Sep 17 00:00:00 2001 From: xeno097 Date: Mon, 21 Oct 2024 18:37:00 -0400 Subject: [PATCH 1/8] feat: cli warp route checker (#4667) ### Description This PR implements the `warp check` command to compare on-chain warp deployments with provided configuration files ### Drive-by changes - updated the `inputFileCommandOption` to be a function for defining cli input file args - defined the `DEFAULT_WARP_ROUTE_DEPLOYMENT_CONFIG_PATH` to avoid hardcoding the `./configs/warp-route-deployment.yaml` file path - implemented the `logCommandHeader` function to format the command header and avoid always having to manually log the `---` separator in command outputs - implements the `getWarpCoreConfigOrExit` to get the warp core configuration from either the registry or a user-defined path and exit early if no input value is provided - Updated the `IsmConfigSchema`s to include the `BaseIsmConfigSchema` because when parsing config files the address field was not included in the parsed object as it wasn't defined on the type Example output ![image](https://github.com/user-attachments/assets/07821a13-d2e2-4b73-b493-9a2c2829a7b3) ![image](https://github.com/user-attachments/assets/768d724f-c96e-4ff5-9c4d-332560c57626) ![image](https://github.com/user-attachments/assets/f92df7c5-acac-4ff7-974b-0334e4a221ab) ### Related issues - Fixes https://github.com/hyperlane-xyz/hyperlane-monorepo/issues/4666 ### Backward compatibility - Yes ### Testing - Manual --- .changeset/grumpy-ears-relate.md | 6 + typescript/cli/src/check/warp.ts | 41 ++++++ typescript/cli/src/commands/config.ts | 8 +- typescript/cli/src/commands/options.ts | 26 +++- typescript/cli/src/commands/warp.ts | 186 ++++++++++--------------- typescript/cli/src/logger.ts | 4 + typescript/cli/src/read/warp.ts | 117 ++++++++++++++++ typescript/cli/src/utils/input.ts | 35 ++++- typescript/cli/src/utils/output.ts | 56 ++++++++ typescript/sdk/src/index.ts | 2 +- typescript/utils/src/index.ts | 2 + typescript/utils/src/objects.test.ts | 136 +++++++++++++++++- typescript/utils/src/objects.ts | 101 ++++++++++++++ 13 files changed, 596 insertions(+), 124 deletions(-) create mode 100644 .changeset/grumpy-ears-relate.md create mode 100644 typescript/cli/src/check/warp.ts create mode 100644 typescript/cli/src/read/warp.ts create mode 100644 typescript/cli/src/utils/output.ts diff --git a/.changeset/grumpy-ears-relate.md b/.changeset/grumpy-ears-relate.md new file mode 100644 index 000000000..8dbc0a515 --- /dev/null +++ b/.changeset/grumpy-ears-relate.md @@ -0,0 +1,6 @@ +--- +'@hyperlane-xyz/cli': minor +'@hyperlane-xyz/sdk': minor +--- + +Adds the warp check command to compare warp routes config files with on chain warp route deployments diff --git a/typescript/cli/src/check/warp.ts b/typescript/cli/src/check/warp.ts new file mode 100644 index 000000000..a31fac62e --- /dev/null +++ b/typescript/cli/src/check/warp.ts @@ -0,0 +1,41 @@ +import { stringify as yamlStringify } from 'yaml'; + +import { WarpRouteDeployConfig, normalizeConfig } from '@hyperlane-xyz/sdk'; +import { ObjectDiff, diffObjMerge } from '@hyperlane-xyz/utils'; + +import { log, logGreen } from '../logger.js'; +import '../utils/output.js'; +import { formatYamlViolationsOutput } from '../utils/output.js'; + +export async function runWarpRouteCheck({ + warpRouteConfig, + onChainWarpConfig, +}: { + warpRouteConfig: WarpRouteDeployConfig; + onChainWarpConfig: WarpRouteDeployConfig; +}): Promise { + // Go through each chain and only add to the output the chains that have mismatches + const [violations, isInvalid] = Object.keys(warpRouteConfig).reduce( + (acc, chain) => { + const { mergedObject, isInvalid } = diffObjMerge( + normalizeConfig(onChainWarpConfig[chain]), + normalizeConfig(warpRouteConfig[chain]), + ); + + if (isInvalid) { + acc[0][chain] = mergedObject; + acc[1] ||= isInvalid; + } + + return acc; + }, + [{}, false] as [{ [index: string]: ObjectDiff }, boolean], + ); + + if (isInvalid) { + log(formatYamlViolationsOutput(yamlStringify(violations, null, 2))); + process.exit(1); + } + + logGreen(`No violations found`); +} diff --git a/typescript/cli/src/commands/config.ts b/typescript/cli/src/commands/config.ts index 7b145ca44..e72b72452 100644 --- a/typescript/cli/src/commands/config.ts +++ b/typescript/cli/src/commands/config.ts @@ -41,7 +41,7 @@ const validateChainCommand: CommandModuleWithContext<{ path: string }> = { command: 'chain', describe: 'Validate a chain config file', builder: { - path: inputFileCommandOption, + path: inputFileCommandOption(), }, handler: async ({ path }) => { readChainConfigs(path); @@ -54,7 +54,7 @@ const validateIsmCommand: CommandModuleWithContext<{ path: string }> = { command: 'ism', describe: 'Validate the basic ISM config file', builder: { - path: inputFileCommandOption, + path: inputFileCommandOption(), }, handler: async ({ path }) => { readMultisigConfig(path); @@ -67,7 +67,7 @@ const validateIsmAdvancedCommand: CommandModuleWithContext<{ path: string }> = { command: 'ism-advanced', describe: 'Validate the advanced ISM config file', builder: { - path: inputFileCommandOption, + path: inputFileCommandOption(), }, handler: async ({ path }) => { readIsmConfig(path); @@ -80,7 +80,7 @@ const validateWarpCommand: CommandModuleWithContext<{ path: string }> = { command: 'warp', describe: 'Validate a Warp Route deployment config file', builder: { - path: inputFileCommandOption, + path: inputFileCommandOption(), }, handler: async ({ path }) => { await readWarpRouteDeployConfig(path); diff --git a/typescript/cli/src/commands/options.ts b/typescript/cli/src/commands/options.ts index a251445be..218671509 100644 --- a/typescript/cli/src/commands/options.ts +++ b/typescript/cli/src/commands/options.ts @@ -91,11 +91,14 @@ export const hookCommandOption: Options = { 'A path to a JSON or YAML file with Hook configs (for every chain)', }; +export const DEFAULT_WARP_ROUTE_DEPLOYMENT_CONFIG_PATH = + './configs/warp-route-deployment.yaml'; + export const warpDeploymentConfigCommandOption: Options = { type: 'string', description: 'A path to a JSON or YAML file with a warp route deployment config.', - default: './configs/warp-route-deployment.yaml', + default: DEFAULT_WARP_ROUTE_DEPLOYMENT_CONFIG_PATH, alias: 'wd', }; @@ -134,12 +137,23 @@ export const outputFileCommandOption = ( demandOption, }); -export const inputFileCommandOption: Options = { +interface InputFileCommandOptionConfig + extends Pick { + defaultPath?: string; +} + +export const inputFileCommandOption = ({ + defaultPath, + demandOption = true, + description = 'Input file path', + alias = 'i', +}: InputFileCommandOptionConfig = {}): Options => ({ type: 'string', - description: 'Input file path', - alias: 'i', - demandOption: true, -}; + description, + default: defaultPath, + alias, + demandOption, +}); export const fromAddressCommandOption: Options = { type: 'string', diff --git a/typescript/cli/src/commands/warp.ts b/typescript/cli/src/commands/warp.ts index 107db6850..b7fb45624 100644 --- a/typescript/cli/src/commands/warp.ts +++ b/typescript/cli/src/commands/warp.ts @@ -1,24 +1,11 @@ -import { ethers } from 'ethers'; import { stringify as yamlStringify } from 'yaml'; import { CommandModule } from 'yargs'; -import { - HypXERC20Lockbox__factory, - HypXERC20__factory, - IXERC20__factory, -} from '@hyperlane-xyz/core'; -import { - ChainMap, - ChainSubmissionStrategySchema, - EvmERC20WarpRouteReader, - TokenStandard, - WarpCoreConfig, -} from '@hyperlane-xyz/sdk'; -import { objMap, promiseObjAll } from '@hyperlane-xyz/utils'; +import { ChainSubmissionStrategySchema } from '@hyperlane-xyz/sdk'; +import { runWarpRouteCheck } from '../check/warp.js'; import { createWarpRouteDeployConfig, - readWarpCoreConfig, readWarpRouteDeployConfig, } from '../config/warp.js'; import { @@ -27,7 +14,8 @@ import { } from '../context/types.js'; import { evaluateIfDryRunFailure } from '../deploy/dry-run.js'; import { runWarpRouteApply, runWarpRouteDeploy } from '../deploy/warp.js'; -import { log, logGray, logGreen, logRed, logTable } from '../logger.js'; +import { log, logCommandHeader, logGreen } from '../logger.js'; +import { runWarpRouteRead } from '../read/warp.js'; import { sendTestTransfer } from '../send/transfer.js'; import { indentYamlOrJson, @@ -35,13 +23,15 @@ import { removeEndingSlash, writeYamlOrJson, } from '../utils/files.js'; -import { selectRegistryWarpRoute } from '../utils/tokens.js'; +import { getWarpCoreConfigOrExit } from '../utils/input.js'; import { + DEFAULT_WARP_ROUTE_DEPLOYMENT_CONFIG_PATH, addressCommandOption, chainCommandOption, dryRunCommandOption, fromAddressCommandOption, + inputFileCommandOption, outputFileCommandOption, strategyCommandOption, symbolCommandOption, @@ -59,6 +49,7 @@ export const warpCommand: CommandModule = { builder: (yargs) => yargs .command(apply) + .command(check) .command(deploy) .command(init) .command(read) @@ -104,17 +95,14 @@ export const apply: CommandModuleWithWriteContext<{ strategy: strategyUrl, receiptsDir, }) => { - logGray(`Hyperlane Warp Apply`); - logGray('--------------------'); // @TODO consider creating a helper function for these dashes - let warpCoreConfig: WarpCoreConfig; - if (symbol) { - warpCoreConfig = await selectRegistryWarpRoute(context.registry, symbol); - } else if (warp) { - warpCoreConfig = readWarpCoreConfig(warp); - } else { - logRed(`Please specify either a symbol or warp config`); - process.exit(0); - } + logCommandHeader('Hyperlane Warp Apply'); + + const warpCoreConfig = await getWarpCoreConfigOrExit({ + symbol, + warp, + context, + }); + if (strategyUrl) ChainSubmissionStrategySchema.parse(readYamlOrJson(strategyUrl)); const warpDeployConfig = await readWarpRouteDeployConfig(config); @@ -143,8 +131,9 @@ export const deploy: CommandModuleWithWriteContext<{ 'from-address': fromAddressCommandOption, }, handler: async ({ context, config, dryRun }) => { - logGray(`Hyperlane Warp Route Deployment${dryRun ? ' Dry-Run' : ''}`); - logGray('------------------------------------------------'); + logCommandHeader( + `Hyperlane Warp Route Deployment${dryRun ? ' Dry-Run' : ''}`, + ); try { await runWarpRouteDeploy({ @@ -171,11 +160,10 @@ export const init: CommandModuleWithContext<{ describe: 'Create an advanced ISM', default: false, }, - out: outputFileCommandOption('./configs/warp-route-deployment.yaml'), + out: outputFileCommandOption(DEFAULT_WARP_ROUTE_DEPLOYMENT_CONFIG_PATH), }, handler: async ({ context, advanced, out }) => { - logGray('Hyperlane Warp Configure'); - logGray('------------------------'); + logCommandHeader('Hyperlane Warp Configure'); await createWarpRouteDeployConfig({ context, @@ -208,7 +196,7 @@ export const read: CommandModuleWithContext<{ false, ), config: outputFileCommandOption( - './configs/warp-route-deployment.yaml', + DEFAULT_WARP_ROUTE_DEPLOYMENT_CONFIG_PATH, false, 'The path to output a Warp Config JSON or YAML file.', ), @@ -220,75 +208,14 @@ export const read: CommandModuleWithContext<{ config: configFilePath, symbol, }) => { - logGray('Hyperlane Warp Reader'); - logGray('---------------------'); - - const { multiProvider } = context; - - let addresses: ChainMap; - if (symbol) { - const warpCoreConfig = await selectRegistryWarpRoute( - context.registry, - symbol, - ); - - // TODO: merge with XERC20TokenAdapter and WarpRouteReader - const xerc20Limits = await Promise.all( - warpCoreConfig.tokens - .filter( - (t) => - t.standard === TokenStandard.EvmHypXERC20 || - t.standard === TokenStandard.EvmHypXERC20Lockbox, - ) - .map(async (t) => { - const provider = multiProvider.getProvider(t.chainName); - const router = t.addressOrDenom!; - const xerc20Address = - t.standard === TokenStandard.EvmHypXERC20Lockbox - ? await HypXERC20Lockbox__factory.connect( - router, - provider, - ).xERC20() - : await HypXERC20__factory.connect( - router, - provider, - ).wrappedToken(); + logCommandHeader('Hyperlane Warp Reader'); - const xerc20 = IXERC20__factory.connect(xerc20Address, provider); - const mint = await xerc20.mintingCurrentLimitOf(router); - const burn = await xerc20.burningCurrentLimitOf(router); - - const formattedLimits = objMap({ mint, burn }, (_, v) => - ethers.utils.formatUnits(v, t.decimals), - ); - - return [t.chainName, formattedLimits]; - }), - ); - if (xerc20Limits.length > 0) { - logGray('xERC20 Limits:'); - logTable(Object.fromEntries(xerc20Limits)); - } - - addresses = Object.fromEntries( - warpCoreConfig.tokens.map((t) => [t.chainName, t.addressOrDenom!]), - ); - } else if (chain && address) { - addresses = { - [chain]: address, - }; - } else { - logGreen(`Please specify either a symbol or chain and address`); - process.exit(0); - } - - const config = await promiseObjAll( - objMap(addresses, async (chain, address) => - new EvmERC20WarpRouteReader(multiProvider, chain).deriveWarpRouteConfig( - address, - ), - ), - ); + const config = await runWarpRouteRead({ + context, + chain, + address, + symbol, + }); if (configFilePath) { writeYamlOrJson(configFilePath, config, 'yaml'); @@ -346,15 +273,11 @@ const send: CommandModuleWithWriteContext< amount, recipient, }) => { - let warpCoreConfig: WarpCoreConfig; - if (symbol) { - warpCoreConfig = await selectRegistryWarpRoute(context.registry, symbol); - } else if (warp) { - warpCoreConfig = readWarpCoreConfig(warp); - } else { - logRed(`Please specify either a symbol or warp config`); - process.exit(0); - } + const warpCoreConfig = await getWarpCoreConfigOrExit({ + symbol, + warp, + context, + }); await sendTestTransfer({ context, @@ -370,3 +293,44 @@ const send: CommandModuleWithWriteContext< process.exit(0); }, }; + +export const check: CommandModuleWithContext<{ + config: string; + symbol?: string; + warp?: string; +}> = { + command: 'check', + describe: + 'Verifies that a warp route configuration matches the on chain configuration.', + builder: { + symbol: { + ...symbolCommandOption, + demandOption: false, + }, + warp: { + ...warpCoreConfigCommandOption, + demandOption: false, + }, + config: inputFileCommandOption({ + defaultPath: DEFAULT_WARP_ROUTE_DEPLOYMENT_CONFIG_PATH, + description: 'The path to a warp route deployment configuration file', + }), + }, + handler: async ({ context, config, symbol, warp }) => { + logCommandHeader('Hyperlane Warp Check'); + + const warpRouteConfig = await readWarpRouteDeployConfig(config, context); + const onChainWarpConfig = await runWarpRouteRead({ + context, + warp, + symbol, + }); + + await runWarpRouteCheck({ + onChainWarpConfig, + warpRouteConfig, + }); + + process.exit(0); + }, +}; diff --git a/typescript/cli/src/logger.ts b/typescript/cli/src/logger.ts index d5347c66d..621d70e4d 100644 --- a/typescript/cli/src/logger.ts +++ b/typescript/cli/src/logger.ts @@ -57,5 +57,9 @@ export const errorRed = (...args: any) => logColor('error', chalk.red, ...args); export const logDebug = (msg: string, ...args: any) => logger.debug(msg, ...args); +export const logCommandHeader = (msg: string) => { + logGray(`${msg}\n${'_'.repeat(msg.length)}`); +}; + // No support for table in pino so print directly to console export const logTable = (...args: any) => console.table(...args); diff --git a/typescript/cli/src/read/warp.ts b/typescript/cli/src/read/warp.ts new file mode 100644 index 000000000..9139d890c --- /dev/null +++ b/typescript/cli/src/read/warp.ts @@ -0,0 +1,117 @@ +import { ethers } from 'ethers'; + +import { + HypXERC20Lockbox__factory, + HypXERC20__factory, + IXERC20__factory, +} from '@hyperlane-xyz/core'; +import { + ChainMap, + ChainName, + EvmERC20WarpRouteReader, + TokenStandard, +} from '@hyperlane-xyz/sdk'; +import { isAddressEvm, objMap, promiseObjAll } from '@hyperlane-xyz/utils'; + +import { CommandContext } from '../context/types.js'; +import { logGray, logRed, logTable } from '../logger.js'; +import { getWarpCoreConfigOrExit } from '../utils/input.js'; + +export async function runWarpRouteRead({ + context, + chain, + address, + warp, + symbol, +}: { + context: CommandContext; + chain?: ChainName; + warp?: string; + address?: string; + symbol?: string; +}): Promise> { + const { multiProvider } = context; + + let addresses: ChainMap; + if (symbol || warp) { + const warpCoreConfig = await getWarpCoreConfigOrExit({ + context, + warp, + symbol, + }); + + // TODO: merge with XERC20TokenAdapter and WarpRouteReader + const xerc20Limits = await Promise.all( + warpCoreConfig.tokens + .filter( + (t) => + t.standard === TokenStandard.EvmHypXERC20 || + t.standard === TokenStandard.EvmHypXERC20Lockbox, + ) + .map(async (t) => { + const provider = multiProvider.getProvider(t.chainName); + const router = t.addressOrDenom!; + const xerc20Address = + t.standard === TokenStandard.EvmHypXERC20Lockbox + ? await HypXERC20Lockbox__factory.connect( + router, + provider, + ).xERC20() + : await HypXERC20__factory.connect( + router, + provider, + ).wrappedToken(); + + const xerc20 = IXERC20__factory.connect(xerc20Address, provider); + const mint = await xerc20.mintingCurrentLimitOf(router); + const burn = await xerc20.burningCurrentLimitOf(router); + + const formattedLimits = objMap({ mint, burn }, (_, v) => + ethers.utils.formatUnits(v, t.decimals), + ); + + return [t.chainName, formattedLimits]; + }), + ); + + if (xerc20Limits.length > 0) { + logGray('xERC20 Limits:'); + logTable(Object.fromEntries(xerc20Limits)); + } + + addresses = Object.fromEntries( + warpCoreConfig.tokens.map((t) => [t.chainName, t.addressOrDenom!]), + ); + } else if (chain && address) { + addresses = { + [chain]: address, + }; + } else { + logRed(`Please specify either a symbol, chain and address or warp file`); + process.exit(1); + } + + // Check if there any non-EVM chains in the config and exit + const nonEvmChains = Object.entries(addresses) + .filter(([_, address]) => !isAddressEvm(address)) + .map(([chain]) => chain); + if (nonEvmChains.length > 0) { + const chainList = nonEvmChains.join(', '); + logRed( + `${chainList} ${ + nonEvmChains.length > 1 ? 'are' : 'is' + } non-EVM and not compatible with the cli`, + ); + process.exit(1); + } + + const config = await promiseObjAll( + objMap(addresses, async (chain, address) => + new EvmERC20WarpRouteReader(multiProvider, chain).deriveWarpRouteConfig( + address, + ), + ), + ); + + return config; +} diff --git a/typescript/cli/src/utils/input.ts b/typescript/cli/src/utils/input.ts index 4b54c4f3e..251c6c0c5 100644 --- a/typescript/cli/src/utils/input.ts +++ b/typescript/cli/src/utils/input.ts @@ -18,9 +18,14 @@ import type { PartialDeep } from '@inquirer/type'; import ansiEscapes from 'ansi-escapes'; import chalk from 'chalk'; -import { logGray } from '../logger.js'; +import { WarpCoreConfig } from '@hyperlane-xyz/sdk'; + +import { readWarpCoreConfig } from '../config/warp.js'; +import { CommandContext } from '../context/types.js'; +import { logGray, logRed } from '../logger.js'; import { indentYamlOrJson } from './files.js'; +import { selectRegistryWarpRoute } from './tokens.js'; export async function detectAndConfirmOrPrompt( detect: () => Promise, @@ -72,6 +77,34 @@ export async function inputWithInfo({ return answer; } +/** + * Gets a {@link WarpCoreConfig} based on the provided path or prompts the user to choose one: + * - if `symbol` is provided the user will have to select one of the available warp routes. + * - if `warp` is provided the config will be read by the provided file path. + * - if none is provided the CLI will exit. + */ +export async function getWarpCoreConfigOrExit({ + context, + symbol, + warp, +}: { + context: CommandContext; + symbol?: string; + warp?: string; +}): Promise { + let warpCoreConfig: WarpCoreConfig; + if (symbol) { + warpCoreConfig = await selectRegistryWarpRoute(context.registry, symbol); + } else if (warp) { + warpCoreConfig = readWarpCoreConfig(warp); + } else { + logRed(`Please specify either a symbol or warp config`); + process.exit(0); + } + + return warpCoreConfig; +} + /** * Searchable checkbox code * diff --git a/typescript/cli/src/utils/output.ts b/typescript/cli/src/utils/output.ts new file mode 100644 index 000000000..442b8a090 --- /dev/null +++ b/typescript/cli/src/utils/output.ts @@ -0,0 +1,56 @@ +import chalk from 'chalk'; + +export enum ViolationDiffType { + None, + Expected, + Actual, +} + +type FormatterByDiffType = Record string>; + +const defaultDiffFormatter: FormatterByDiffType = { + [ViolationDiffType.Actual]: chalk.red, + [ViolationDiffType.Expected]: chalk.green, + [ViolationDiffType.None]: (text: string) => text, +}; + +/** + * Takes a yaml formatted string and highlights differences by looking at `expected` and `actual` properties. + */ +export function formatYamlViolationsOutput( + yamlString: string, + formatters: FormatterByDiffType = defaultDiffFormatter, +): string { + const lines = yamlString.split('\n'); + + let curr: ViolationDiffType = ViolationDiffType.None; + let lastDiffIndent = 0; + const highlightedLines = lines.map((line) => { + // Get how many white space/tabs we have before the property name + const match = line.match(/^(\s*)/); + const currentIndent = match ? match[0].length : 0; + + let formattedLine = line; + // if the current indentation is smaller than the previous diff one + // we just got out of a diff property and we reset the formatting + if (currentIndent < lastDiffIndent) { + curr = ViolationDiffType.None; + } + + if (line.includes('expected:')) { + lastDiffIndent = currentIndent; + curr = ViolationDiffType.Expected; + formattedLine = line.replace('expected:', 'EXPECTED:'); + } + + if (line.includes('actual:')) { + lastDiffIndent = currentIndent; + curr = ViolationDiffType.Actual; + formattedLine = line.replace('actual:', 'ACTUAL:'); + } + + return formatters[curr](formattedLine); + }); + + return highlightedLines.join('\n'); +} diff --git a/typescript/sdk/src/index.ts b/typescript/sdk/src/index.ts index e80ad1f3e..415a11d26 100644 --- a/typescript/sdk/src/index.ts +++ b/typescript/sdk/src/index.ts @@ -487,7 +487,7 @@ export { setFork, stopImpersonatingAccount, } from './utils/fork.js'; -export { multisigIsmVerificationCost } from './utils/ism.js'; +export { multisigIsmVerificationCost, normalizeConfig } from './utils/ism.js'; export { MultiGeneric } from './utils/MultiGeneric.js'; export { SealevelAccountDataWrapper, diff --git a/typescript/utils/src/index.ts b/typescript/utils/src/index.ts index e45994f8f..0c18543dd 100644 --- a/typescript/utils/src/index.ts +++ b/typescript/utils/src/index.ts @@ -119,6 +119,8 @@ export { pick, promiseObjAll, stringifyObject, + diffObjMerge, + ObjectDiff, } from './objects.js'; export { Result, failure, success } from './result.js'; export { difference, setEquality, symmetricDifference } from './sets.js'; diff --git a/typescript/utils/src/objects.test.ts b/typescript/utils/src/objects.test.ts index b6fd1d012..d5ed72a6b 100644 --- a/typescript/utils/src/objects.test.ts +++ b/typescript/utils/src/objects.test.ts @@ -1,6 +1,12 @@ import { expect } from 'chai'; -import { deepCopy, deepEquals, objMerge, objOmit } from './objects.js'; +import { + deepCopy, + deepEquals, + diffObjMerge, + objMerge, + objOmit, +} from './objects.js'; describe('Object utilities', () => { it('deepEquals', () => { @@ -67,4 +73,132 @@ describe('Object utilities', () => { const omitted1_2 = objOmit(obj1, obj2, 10, false); expect(omitted1_2).to.eql({ a: 1, b: { d: 'string' } }); }); + + describe('diffObjMerge', () => { + it('should merge objects with equal values', () => { + const actual = { a: 1, b: 2 }; + const expected = { a: 1, b: 2 }; + + const result = diffObjMerge(actual, expected); + + expect(result).to.eql({ + isInvalid: false, + mergedObject: { a: 1, b: 2 }, + }); + }); + + it('should return a diff for objects with different values', () => { + const actual = { a: 1, b: 2 }; + const expected = { a: 1, b: 3 }; + + const result = diffObjMerge(actual, expected); + + expect(result).to.eql({ + isInvalid: true, + mergedObject: { + a: 1, + b: { actual: 2, expected: 3 }, + }, + }); + }); + + it('should detect missing fields in the top level object', () => { + const actual = { a: 1 }; + const expected = { a: 1, b: 3 }; + + const result = diffObjMerge(actual, expected); + + expect(result).to.eql({ + isInvalid: true, + mergedObject: { + a: 1, + b: { actual: '', expected: 3 }, + }, + }); + }); + + it('should detect extra fields in the top level object', () => { + const actual = { a: 1, b: 2 }; + const expected = { a: 1 }; + + const result = diffObjMerge(actual, expected); + + expect(result).to.eql({ + isInvalid: true, + mergedObject: { + a: 1, + b: { actual: 2, expected: '' }, + }, + }); + }); + + it('should merge nested objects and show differences', () => { + const actual = { a: 1, b: { c: 2, d: 4 } }; + const expected = { a: 1, b: { c: 2, d: 3 } }; + + const result = diffObjMerge(actual, expected); + + expect(result).to.eql({ + isInvalid: true, + mergedObject: { + a: 1, + b: { + c: 2, + d: { actual: 4, expected: 3 }, + }, + }, + }); + }); + + it('should throw an error when maxDepth is exceeded', () => { + const actual = { a: { b: { c: { d: { e: 5 } } } } }; + const expected = { a: { b: { c: { d: { e: 5 } } } } }; + + expect(() => diffObjMerge(actual, expected, 3)).to.Throw( + 'diffObjMerge tried to go too deep', + ); + }); + + it('should merge arrays of equal length and show the diffs', () => { + const actual = [1, 2, 3]; + const expected = [1, 2, 4]; + + const result = diffObjMerge(actual, expected); + + expect(result).to.eql({ + isInvalid: true, + mergedObject: [1, 2, { actual: 3, expected: 4 }], + }); + }); + + it('should return a diff for arrays of different lengths', () => { + const actual = [1, 2]; + const expected = [1, 2, 3]; + + const result = diffObjMerge(actual, expected); + + expect(result).to.eql({ + isInvalid: true, + mergedObject: { + actual, + expected, + }, + }); + }); + + it('should handle null and undefined values properly', () => { + const actual = { a: null, b: 2 }; + const expected = { a: undefined, b: 2 }; + + const result = diffObjMerge(actual, expected); + + expect(result).to.eql({ + isInvalid: false, + mergedObject: { + a: undefined, + b: 2, + }, + }); + }); + }); }); diff --git a/typescript/utils/src/objects.ts b/typescript/utils/src/objects.ts index 680c5d579..403caa849 100644 --- a/typescript/utils/src/objects.ts +++ b/typescript/utils/src/objects.ts @@ -2,6 +2,7 @@ import { cloneDeep, isEqual } from 'lodash-es'; import { stringify as yamlStringify } from 'yaml'; import { ethersBigNumberSerializer } from './logging.js'; +import { isNullish } from './typeof.js'; import { assert } from './validation.js'; export function isObject(item: any) { @@ -216,3 +217,103 @@ export function stringifyObject( } return yamlStringify(JSON.parse(json), null, space); } + +interface ObjectDiffOutput { + actual: any; + expected: any; +} + +export type ObjectDiff = + | { + [key: string]: ObjectDiffOutput | ObjectDiff; + } + | ObjectDiff[] + | undefined; + +/** + * Merges 2 objects showing any difference in value for common fields. + */ +export function diffObjMerge( + actual: Record, + expected: Record, + maxDepth = 10, +): { + mergedObject: ObjectDiff; + isInvalid: boolean; +} { + if (maxDepth === 0) { + throw new Error('diffObjMerge tried to go too deep'); + } + + let isDiff = false; + if (!isObject(actual) && !isObject(expected) && actual === expected) { + return { + isInvalid: isDiff, + mergedObject: actual, + }; + } + + if (isNullish(actual) && isNullish(expected)) { + return { mergedObject: undefined, isInvalid: isDiff }; + } + + if (isObject(actual) && isObject(expected)) { + const ret: Record = {}; + + const actualKeys = new Set(Object.keys(actual)); + const expectedKeys = new Set(Object.keys(expected)); + const allKeys = new Set([...actualKeys, ...expectedKeys]); + for (const key of allKeys.values()) { + if (actualKeys.has(key) && expectedKeys.has(key)) { + const { mergedObject, isInvalid } = + diffObjMerge(actual[key], expected[key], maxDepth - 1) ?? {}; + ret[key] = mergedObject; + isDiff ||= isInvalid; + } else if (actualKeys.has(key) && !isNullish(actual[key])) { + ret[key] = { + actual: actual[key], + expected: '' as any, + }; + isDiff = true; + } else if (!isNullish(expected[key])) { + ret[key] = { + actual: '' as any, + expected: expected[key], + }; + isDiff = true; + } + } + return { + isInvalid: isDiff, + mergedObject: ret, + }; + } + + // Merge the elements of the array to see if there are any differences + if ( + Array.isArray(actual) && + Array.isArray(expected) && + actual.length === expected.length + ) { + const merged = actual.reduce( + (acc: [ObjectDiff[], boolean], curr, idx) => { + const { isInvalid, mergedObject } = diffObjMerge(curr, expected[idx]); + + acc[0].push(mergedObject); + acc[1] ||= isInvalid; + + return acc; + }, + [[], isDiff], + ); + return { + isInvalid: merged[1], + mergedObject: merged[0], + }; + } + + return { + mergedObject: { expected: expected ?? '', actual: actual ?? '' }, + isInvalid: true, + }; +} From c9085afd96c61a74207feff2dc5dbd86ec37ff95 Mon Sep 17 00:00:00 2001 From: Kunal Arora <55632507+aroralanuk@users.noreply.github.com> Date: Tue, 22 Oct 2024 12:38:32 +0530 Subject: [PATCH 2/8] fix: add fix for checking for correct messageId in `OPL2ToL1Ism` and `ArbL2ToL1Ism` via external call (#4437) ### Description - In agreement with Chainlight team's recommendation, added a second isVerified() check after portal call to make sure an arbitrary call which passes the check for metadata length and messageId cannot be verified without calling is verifyMessageId(messageId) in `OPL2ToL1Ism` and `ArbL2ToL1` ### Drive-by changes None ### Related issues - fixes https://github.com/chainlight-io/2024-08-hyperlane/issues/2 ### Backward compatibility No, but the contracts aren't deployed anywhere ### Testing Unit testing --- .changeset/itchy-singers-hang.md | 5 +++++ solidity/contracts/isms/hook/ArbL2ToL1Ism.sol | 1 + solidity/contracts/isms/hook/OPL2ToL1Ism.sol | 4 ++-- solidity/test/isms/ERC5164ISM.t.sol | 2 ++ solidity/test/isms/ExternalBridgeTest.sol | 20 +++++++++++++++++-- solidity/test/isms/OPStackIsm.t.sol | 10 ++++++---- 6 files changed, 34 insertions(+), 8 deletions(-) create mode 100644 .changeset/itchy-singers-hang.md diff --git a/.changeset/itchy-singers-hang.md b/.changeset/itchy-singers-hang.md new file mode 100644 index 000000000..97096ff1a --- /dev/null +++ b/.changeset/itchy-singers-hang.md @@ -0,0 +1,5 @@ +--- +'@hyperlane-xyz/core': patch +--- + +Patched OPL2ToL1Ism to check for correct messageId for external call in verify diff --git a/solidity/contracts/isms/hook/ArbL2ToL1Ism.sol b/solidity/contracts/isms/hook/ArbL2ToL1Ism.sol index a7bd71447..98b5f9bd6 100644 --- a/solidity/contracts/isms/hook/ArbL2ToL1Ism.sol +++ b/solidity/contracts/isms/hook/ArbL2ToL1Ism.sol @@ -63,6 +63,7 @@ contract ArbL2ToL1Ism is ) external override returns (bool) { if (!isVerified(message)) { _verifyWithOutboxCall(metadata, message); + require(isVerified(message), "ArbL2ToL1Ism: message not verified"); } releaseValueToRecipient(message); return true; diff --git a/solidity/contracts/isms/hook/OPL2ToL1Ism.sol b/solidity/contracts/isms/hook/OPL2ToL1Ism.sol index b333b15cd..ef3986861 100644 --- a/solidity/contracts/isms/hook/OPL2ToL1Ism.sol +++ b/solidity/contracts/isms/hook/OPL2ToL1Ism.sol @@ -66,9 +66,9 @@ contract OPL2ToL1Ism is bytes calldata metadata, bytes calldata message ) external override returns (bool) { - bool verified = isVerified(message); - if (!verified) { + if (!isVerified(message)) { _verifyWithPortalCall(metadata, message); + require(isVerified(message), "OPL2ToL1Ism: message not verified"); } releaseValueToRecipient(message); return true; diff --git a/solidity/test/isms/ERC5164ISM.t.sol b/solidity/test/isms/ERC5164ISM.t.sol index 75c79fb82..8063979f4 100644 --- a/solidity/test/isms/ERC5164ISM.t.sol +++ b/solidity/test/isms/ERC5164ISM.t.sol @@ -150,6 +150,8 @@ contract ERC5164IsmTest is ExternalBridgeTest { function test_verify_valueAlreadyClaimed(uint256) public override {} + function test_verify_false_arbitraryCall() public override {} + /* ============ helper functions ============ */ function _externalBridgeDestinationCall( diff --git a/solidity/test/isms/ExternalBridgeTest.sol b/solidity/test/isms/ExternalBridgeTest.sol index 8db043fcf..344e001af 100644 --- a/solidity/test/isms/ExternalBridgeTest.sol +++ b/solidity/test/isms/ExternalBridgeTest.sol @@ -135,14 +135,14 @@ abstract contract ExternalBridgeTest is Test { 1 ether, messageId ); - ism.verify(externalCalldata, encodedMessage); + assertTrue(ism.verify(externalCalldata, encodedMessage)); assertEq(address(testRecipient).balance, 1 ether); } function test_verify_revertsWhen_invalidIsm() public virtual { bytes memory externalCalldata = _encodeExternalDestinationBridgeCall( address(hook), - address(this), + address(hook), 0, messageId ); @@ -217,6 +217,19 @@ abstract contract ExternalBridgeTest is Test { assertEq(address(testRecipient).balance, _msgValue); } + function test_verify_false_arbitraryCall() public virtual { + bytes memory incorrectCalldata = _encodeExternalDestinationBridgeCall( + address(hook), + address(this), + 0, + messageId + ); + + vm.expectRevert(); + ism.verify(incorrectCalldata, encodedMessage); + assertFalse(ism.isVerified(encodedMessage)); + } + /* ============ helper functions ============ */ function _encodeTestMessage() internal view returns (bytes memory) { @@ -265,4 +278,7 @@ abstract contract ExternalBridgeTest is Test { function _setExternalOriginSender( address _sender ) internal virtual returns (bytes memory) {} + + // meant to mock an arbitrary successful call made by the external bridge + function verifyMessageId(bytes32 /*messageId*/) public payable {} } diff --git a/solidity/test/isms/OPStackIsm.t.sol b/solidity/test/isms/OPStackIsm.t.sol index 45c818ec3..3230e59b8 100644 --- a/solidity/test/isms/OPStackIsm.t.sol +++ b/solidity/test/isms/OPStackIsm.t.sol @@ -133,10 +133,10 @@ contract OPStackIsmTest is ExternalBridgeTest { } function _encodeExternalDestinationBridgeCall( - address _from, - address _to, - uint256 _msgValue, - bytes32 _messageId + address /*_from*/, + address /*_to*/, + uint256 /*_msgValue*/, + bytes32 /*_messageId*/ ) internal pure override returns (bytes memory) { return new bytes(0); } @@ -148,6 +148,8 @@ contract OPStackIsmTest is ExternalBridgeTest { function test_verify_revertsWhen_invalidIsm() public override {} + function test_verify_false_arbitraryCall() public override {} + /* ============ ISM.verifyMessageId ============ */ function test_verify_revertsWhen_notAuthorizedHook() public override { From 2760da1ded71159d8b681b75a3d2a797586e81d4 Mon Sep 17 00:00:00 2001 From: Kunal Arora <55632507+aroralanuk@users.noreply.github.com> Date: Tue, 22 Oct 2024 12:40:09 +0530 Subject: [PATCH 3/8] fix: ensure no duplicate signatures verified for `AbstractWeightedMultisigIsm.verify()` (#4468) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit ### Description - There's a bug in the verify function of `AbstractWeightedMultisigIsm` that an attacker might use to bypass the verification of multiple signatures. The code tried to check the duplication of the signers if not found, but the code does not increment validatorIndex when the recovered signer matches to the stored signer. For instance: - validatorsAndThresholdWeight returns [A, B, C, D] - an attacker uses signatures as [sig from A, sig from A, sig from A, sig from A, ...] or [sig from B, sig from B, …] Fix is to add a ++validatorIndex at the end of the for loop implies we don't allow the next signer to be the same as the signer we just verified. ### Drive-by changes None ### Related issues From Chainlight's audit findings ### Backward compatibility We haven't deployed these contracts yet on testnet/mainnet ### Testing Fuzz testing --- .../multisig/AbstractWeightedMultisigIsm.sol | 12 ++- solidity/test/isms/MultisigIsm.t.sol | 26 +++++++ solidity/test/isms/WeightedMultisigIsm.t.sol | 75 ++++++++++++++++++- 3 files changed, 107 insertions(+), 6 deletions(-) diff --git a/solidity/contracts/isms/multisig/AbstractWeightedMultisigIsm.sol b/solidity/contracts/isms/multisig/AbstractWeightedMultisigIsm.sol index 2264b192c..8b7fee049 100644 --- a/solidity/contracts/isms/multisig/AbstractWeightedMultisigIsm.sol +++ b/solidity/contracts/isms/multisig/AbstractWeightedMultisigIsm.sol @@ -73,11 +73,14 @@ abstract contract AbstractStaticWeightedMultisigIsm is // assumes that signatures are ordered by validator for ( - uint256 i = 0; - _totalWeight < _thresholdWeight && i < _validatorCount; - ++i + uint256 signatureIndex = 0; + _totalWeight < _thresholdWeight && signatureIndex < _validatorCount; + ++signatureIndex ) { - address _signer = ECDSA.recover(_digest, signatureAt(_metadata, i)); + address _signer = ECDSA.recover( + _digest, + signatureAt(_metadata, signatureIndex) + ); // loop through remaining validators until we find a match while ( _validatorIndex < _validatorCount && @@ -90,6 +93,7 @@ abstract contract AbstractStaticWeightedMultisigIsm is // add the weight of the current validator _totalWeight += _validators[_validatorIndex].weight; + ++_validatorIndex; } require( _totalWeight >= _thresholdWeight, diff --git a/solidity/test/isms/MultisigIsm.t.sol b/solidity/test/isms/MultisigIsm.t.sol index 8c0b61d38..29098475f 100644 --- a/solidity/test/isms/MultisigIsm.t.sol +++ b/solidity/test/isms/MultisigIsm.t.sol @@ -186,6 +186,32 @@ abstract contract AbstractMultisigIsmTest is Test { metadata[index] = ~metadata[index]; assertFalse(ism.verify(metadata, message)); } + + function test_verify_revertWhen_duplicateSignatures( + uint32 destination, + bytes32 recipient, + bytes calldata body, + uint8 m, + uint8 n, + bytes32 seed + ) public virtual { + vm.assume(1 < m && m <= n && n < 10); + bytes memory message = getMessage(destination, recipient, body); + bytes memory metadata = getMetadata(m, n, seed, message); + + bytes memory duplicateMetadata = new bytes(metadata.length); + for (uint256 i = 0; i < metadata.length - 65; i++) { + duplicateMetadata[i] = metadata[i]; + } + for (uint256 i = 0; i < 65; i++) { + duplicateMetadata[metadata.length - 65 + i] = metadata[ + metadata.length - 130 + i + ]; + } + + vm.expectRevert("!threshold"); + ism.verify(duplicateMetadata, message); + } } contract MerkleRootMultisigIsmTest is AbstractMultisigIsmTest { diff --git a/solidity/test/isms/WeightedMultisigIsm.t.sol b/solidity/test/isms/WeightedMultisigIsm.t.sol index df2a3d0ea..0c5fd7ee4 100644 --- a/solidity/test/isms/WeightedMultisigIsm.t.sol +++ b/solidity/test/isms/WeightedMultisigIsm.t.sol @@ -65,7 +65,6 @@ abstract contract AbstractStaticWeightedMultisigIsmTest is } } - // ism = IInterchainSecurityModule(deployedIsm); ism = IInterchainSecurityModule( weightedFactory.deploy(validators, threshold) ); @@ -136,7 +135,7 @@ abstract contract AbstractStaticWeightedMultisigIsmTest is return metadata; } - function testVerify_revertInsufficientWeight( + function test_verify_revertInsufficientWeight( uint32 destination, bytes32 recipient, bytes calldata body, @@ -161,6 +160,34 @@ abstract contract AbstractStaticWeightedMultisigIsmTest is vm.expectRevert("Insufficient validator weight"); ism.verify(insufficientMetadata, message); } + + function test_verify_revertWhen_duplicateSignatures( + uint32 destination, + bytes32 recipient, + bytes calldata body, + uint8 m, + uint8 n, + bytes32 seed + ) public virtual override { + vm.assume(1 < m && m <= n && n < 10); + bytes memory message = getMessage(destination, recipient, body); + bytes memory metadata = getMetadata(m, n, seed, message); + + bytes memory duplicateMetadata = new bytes(metadata.length); + for (uint256 i = 0; i < metadata.length - 65; i++) { + duplicateMetadata[i] = metadata[i]; + } + for (uint256 i = 0; i < 65; i++) { + duplicateMetadata[metadata.length - 65 + i] = metadata[ + metadata.length - 130 + i + ]; + } + + if (weightedIsm.signatureCount(metadata) >= 2) { + vm.expectRevert("Invalid signer"); + ism.verify(duplicateMetadata, message); + } + } } contract StaticMerkleRootWeightedMultisigIsmTest is @@ -194,6 +221,28 @@ contract StaticMerkleRootWeightedMultisigIsmTest is message ); } + + function test_verify_revertWhen_duplicateSignatures( + uint32 destination, + bytes32 recipient, + bytes calldata body, + uint8 m, + uint8 n, + bytes32 seed + ) + public + override(AbstractMultisigIsmTest, AbstractStaticWeightedMultisigIsmTest) + { + AbstractStaticWeightedMultisigIsmTest + .test_verify_revertWhen_duplicateSignatures( + destination, + recipient, + body, + m, + n, + seed + ); + } } contract StaticMessageIdWeightedMultisigIsmTest is @@ -227,4 +276,26 @@ contract StaticMessageIdWeightedMultisigIsmTest is message ); } + + function test_verify_revertWhen_duplicateSignatures( + uint32 destination, + bytes32 recipient, + bytes calldata body, + uint8 m, + uint8 n, + bytes32 seed + ) + public + override(AbstractMultisigIsmTest, AbstractStaticWeightedMultisigIsmTest) + { + AbstractStaticWeightedMultisigIsmTest + .test_verify_revertWhen_duplicateSignatures( + destination, + recipient, + body, + m, + n, + seed + ); + } } From ec6b874b15fcdee8ddbd189fb714866f2c5df6be Mon Sep 17 00:00:00 2001 From: Kunal Arora <55632507+aroralanuk@users.noreply.github.com> Date: Tue, 22 Oct 2024 12:42:06 +0530 Subject: [PATCH 4/8] feat(contracts): add nonce for monotonically increasing delivery ordering for `HypERC4626` (#4534) ### Description - Added a `rateUpdateNonce` in `HypERC4626Collateral` and `previousNonce` in `HypERC4626` to ensure we only update the exchangeRate on the synthetic asset if the update was after the last recorded update. This is to make sure we don't update it to a stale exchange rate which may cause losses to users using the synthetic asset. ### Drive-by changes - `processInboundMessage` in MockMailbox` to simulate processing of messages out of order. ### Related issues - fixes https://github.com/chainlight-io/2024-08-hyperlane/issues/12 ### Backward compatibility Yes ### Testing Unit test --- .changeset/red-actors-shop.md | 5 ++ solidity/contracts/mock/MockMailbox.sol | 5 ++ .../contracts/token/extensions/HypERC4626.sol | 14 +++- .../token/extensions/HypERC4626Collateral.sol | 9 ++- solidity/test/token/HypERC4626Test.t.sol | 69 +++++++++++++++++-- 5 files changed, 95 insertions(+), 7 deletions(-) create mode 100644 .changeset/red-actors-shop.md diff --git a/.changeset/red-actors-shop.md b/.changeset/red-actors-shop.md new file mode 100644 index 000000000..0ee301e90 --- /dev/null +++ b/.changeset/red-actors-shop.md @@ -0,0 +1,5 @@ +--- +'@hyperlane-xyz/core': patch +--- + +Added nonce to HypERC4626 diff --git a/solidity/contracts/mock/MockMailbox.sol b/solidity/contracts/mock/MockMailbox.sol index ad212dcef..c4b4b63e9 100644 --- a/solidity/contracts/mock/MockMailbox.sol +++ b/solidity/contracts/mock/MockMailbox.sol @@ -77,4 +77,9 @@ contract MockMailbox is Mailbox { Mailbox(address(this)).process{value: msg.value}("", _message); inboundProcessedNonce++; } + + function processInboundMessage(uint32 _nonce) public { + bytes memory _message = inboundMessages[_nonce]; + Mailbox(address(this)).process("", _message); + } } diff --git a/solidity/contracts/token/extensions/HypERC4626.sol b/solidity/contracts/token/extensions/HypERC4626.sol index 2252696fa..141b081ba 100644 --- a/solidity/contracts/token/extensions/HypERC4626.sol +++ b/solidity/contracts/token/extensions/HypERC4626.sol @@ -17,9 +17,12 @@ contract HypERC4626 is HypERC20 { using Message for bytes; using TokenMessage for bytes; + event ExchangeRateUpdated(uint256 newExchangeRate, uint32 rateUpdateNonce); + uint256 public constant PRECISION = 1e10; uint32 public immutable collateralDomain; uint256 public exchangeRate; // 1e10 + uint32 public previousNonce; constructor( uint8 _decimals, @@ -66,7 +69,16 @@ contract HypERC4626 is HypERC20 { bytes calldata _message ) internal virtual override { if (_origin == collateralDomain) { - exchangeRate = abi.decode(_message.metadata(), (uint256)); + (uint256 newExchangeRate, uint32 rateUpdateNonce) = abi.decode( + _message.metadata(), + (uint256, uint32) + ); + // only update if the nonce is greater than the previous nonce + if (rateUpdateNonce > previousNonce) { + exchangeRate = newExchangeRate; + previousNonce = rateUpdateNonce; + emit ExchangeRateUpdated(exchangeRate, rateUpdateNonce); + } } super._handle(_origin, _sender, _message); } diff --git a/solidity/contracts/token/extensions/HypERC4626Collateral.sol b/solidity/contracts/token/extensions/HypERC4626Collateral.sol index 8a084134c..0ae32ab2f 100644 --- a/solidity/contracts/token/extensions/HypERC4626Collateral.sol +++ b/solidity/contracts/token/extensions/HypERC4626Collateral.sol @@ -20,6 +20,8 @@ contract HypERC4626Collateral is HypERC20Collateral { uint256 public constant PRECISION = 1e10; bytes32 public constant NULL_RECIPIENT = 0x0000000000000000000000000000000000000000000000000000000000000001; + // Nonce for the rate update, to ensure sequential updates + uint32 public rateUpdateNonce; constructor( ERC4626 _vault, @@ -52,7 +54,12 @@ contract HypERC4626Collateral is HypERC20Collateral { vault.totalSupply(), Math.Rounding.Down ); - bytes memory _tokenMetadata = abi.encode(_exchangeRate); + + rateUpdateNonce++; + bytes memory _tokenMetadata = abi.encode( + _exchangeRate, + rateUpdateNonce + ); bytes memory _tokenMessage = TokenMessage.format( _recipient, diff --git a/solidity/test/token/HypERC4626Test.t.sol b/solidity/test/token/HypERC4626Test.t.sol index d09e0aae6..338d39b75 100644 --- a/solidity/test/token/HypERC4626Test.t.sol +++ b/solidity/test/token/HypERC4626Test.t.sol @@ -43,6 +43,8 @@ contract HypERC4626CollateralTest is HypTokenTest { HypERC4626 remoteRebasingToken; HypERC4626 peerRebasingToken; + event ExchangeRateUpdated(uint256 newExchangeRate, uint32 rateUpdateNonce); + function setUp() public override { super.setUp(); @@ -95,6 +97,7 @@ contract HypERC4626CollateralTest is HypTokenTest { peerRebasingToken = HypERC4626(address(peerToken)); primaryToken.transfer(ALICE, 1000e18); + primaryToken.transfer(BOB, 1000e18); uint32[] memory domains = new uint32[](3); domains[0] = ORIGIN; @@ -146,6 +149,47 @@ contract HypERC4626CollateralTest is HypTokenTest { ); } + function testRebase_exchangeRateUpdateInSequence() public { + _performRemoteTransferWithoutExpectation(0, transferAmount); + _accrueYield(); + + uint256 exchangeRateInitially = remoteRebasingToken.exchangeRate(); + + vm.startPrank(BOB); + primaryToken.approve(address(localToken), transferAmount); + localToken.transferRemote( + DESTINATION, + BOB.addressToBytes32(), + transferAmount + ); + vm.stopPrank(); + + _accrueYield(); + + vm.startPrank(ALICE); + primaryToken.approve(address(localToken), transferAmount); + localToken.transferRemote( + DESTINATION, + BOB.addressToBytes32(), + transferAmount + ); + vm.stopPrank(); + + // process ALICE's transfer + + vm.expectEmit(true, true, true, true); + emit ExchangeRateUpdated(10721400472, 3); + remoteMailbox.processInboundMessage(2); + uint256 exchangeRateBefore = remoteRebasingToken.exchangeRate(); + + // process BOB's transfer + remoteMailbox.processInboundMessage(1); + uint256 exchangeRateAfter = remoteRebasingToken.exchangeRate(); + + assertLt(exchangeRateInitially, exchangeRateBefore); // updates bc nonce=2 is after nonce=0 + assertEq(exchangeRateBefore, exchangeRateAfter); // doesn't update bc nonce=1 is before nonce=0 + } + function testSyntheticTransfers_withRebase() public { _performRemoteTransferWithoutExpectation(0, transferAmount); assertEq(remoteToken.balanceOf(BOB), transferAmount); @@ -173,6 +217,7 @@ contract HypERC4626CollateralTest is HypTokenTest { } function testWithdrawalWithoutYield() public { + uint256 bobPrimaryBefore = primaryToken.balanceOf(BOB); _performRemoteTransferWithoutExpectation(0, transferAmount); assertEq(remoteToken.balanceOf(BOB), transferAmount); @@ -183,10 +228,14 @@ contract HypERC4626CollateralTest is HypTokenTest { transferAmount ); localMailbox.processNextInboundMessage(); - assertEq(primaryToken.balanceOf(BOB), transferAmount); + assertEq( + primaryToken.balanceOf(BOB) - bobPrimaryBefore, + transferAmount + ); } function testWithdrawalWithYield() public { + uint256 bobPrimaryBefore = primaryToken.balanceOf(BOB); _performRemoteTransferWithoutExpectation(0, transferAmount); assertEq(remoteToken.balanceOf(BOB), transferAmount); @@ -205,13 +254,22 @@ contract HypERC4626CollateralTest is HypTokenTest { uint256 _expectedBal = transferAmount + _discountedYield(); // BOB gets the yield even though it didn't rebase - assertApproxEqRelDecimal(_bobBal, _expectedBal, 1e14, 0); - assertTrue(_bobBal < _expectedBal, "Transfer remote should round down"); + assertApproxEqRelDecimal( + _bobBal - bobPrimaryBefore, + _expectedBal, + 1e14, + 0 + ); + assertTrue( + _bobBal - bobPrimaryBefore < _expectedBal, + "Transfer remote should round down" + ); assertEq(vault.accumulatedFees(), YIELD / 10); } function testWithdrawalAfterYield() public { + uint256 bobPrimaryBefore = primaryToken.balanceOf(BOB); _performRemoteTransferWithoutExpectation(0, transferAmount); assertEq(remoteToken.balanceOf(BOB), transferAmount); @@ -230,7 +288,7 @@ contract HypERC4626CollateralTest is HypTokenTest { ); localMailbox.processNextInboundMessage(); assertApproxEqRelDecimal( - primaryToken.balanceOf(BOB), + primaryToken.balanceOf(BOB) - bobPrimaryBefore, transferAmount + _discountedYield(), 1e14, 0 @@ -287,6 +345,7 @@ contract HypERC4626CollateralTest is HypTokenTest { } function testWithdrawalAfterDrawdown() public { + uint256 bobPrimaryBefore = primaryToken.balanceOf(BOB); _performRemoteTransferWithoutExpectation(0, transferAmount); assertEq(remoteToken.balanceOf(BOB), transferAmount); @@ -306,7 +365,7 @@ contract HypERC4626CollateralTest is HypTokenTest { ); localMailbox.processNextInboundMessage(); assertApproxEqRelDecimal( - primaryToken.balanceOf(BOB), + primaryToken.balanceOf(BOB) - bobPrimaryBefore, transferAmount - drawdown, 1e14, 0 From 3e1ab756433479602e719fd0cd56979156cbde78 Mon Sep 17 00:00:00 2001 From: Kunal Arora <55632507+aroralanuk@users.noreply.github.com> Date: Tue, 22 Oct 2024 12:44:38 +0530 Subject: [PATCH 5/8] fix(contracts): add check for valid mailbox and relayer in `TrustedRelayerIsm` (#4553) ### Description - minor fix: add an isContract check for mailbox and non-zero check for relayer when instantiating TrustedRelayerIsm ### Drive-by changes None ### Related issues - partly fixes https://github.com/chainlight-io/2024-08-hyperlane/issues/14 ### Backward compatibility Yes ### Testing Unit --- solidity/contracts/isms/TrustedRelayerIsm.sol | 9 +++++++++ solidity/test/isms/TrustedRelayerIsm.t.sol | 7 +++++++ 2 files changed, 16 insertions(+) diff --git a/solidity/contracts/isms/TrustedRelayerIsm.sol b/solidity/contracts/isms/TrustedRelayerIsm.sol index aba894a94..87da1bb60 100644 --- a/solidity/contracts/isms/TrustedRelayerIsm.sol +++ b/solidity/contracts/isms/TrustedRelayerIsm.sol @@ -3,6 +3,7 @@ pragma solidity >=0.8.0; // ============ Internal Imports ============ import {IInterchainSecurityModule} from "../interfaces/IInterchainSecurityModule.sol"; +import {Address} from "@openzeppelin/contracts/utils/Address.sol"; import {Message} from "../libs/Message.sol"; import {Mailbox} from "../Mailbox.sol"; import {PackageVersioned} from "contracts/PackageVersioned.sol"; @@ -15,6 +16,14 @@ contract TrustedRelayerIsm is IInterchainSecurityModule, PackageVersioned { address public immutable trustedRelayer; constructor(address _mailbox, address _trustedRelayer) { + require( + _trustedRelayer != address(0), + "TrustedRelayerIsm: invalid relayer" + ); + require( + Address.isContract(_mailbox), + "TrustedRelayerIsm: invalid mailbox" + ); mailbox = Mailbox(_mailbox); trustedRelayer = _trustedRelayer; } diff --git a/solidity/test/isms/TrustedRelayerIsm.t.sol b/solidity/test/isms/TrustedRelayerIsm.t.sol index 51c574ba1..f630b6474 100644 --- a/solidity/test/isms/TrustedRelayerIsm.t.sol +++ b/solidity/test/isms/TrustedRelayerIsm.t.sol @@ -29,6 +29,13 @@ contract TrustedRelayerIsmTest is Test { recipient.setInterchainSecurityModule(address(ism)); } + function test_revertsWhen_invalidMailboxOrRelayer() public { + vm.expectRevert("TrustedRelayerIsm: invalid relayer"); + new TrustedRelayerIsm(address(mailbox), address(0)); + vm.expectRevert("TrustedRelayerIsm: invalid mailbox"); + new TrustedRelayerIsm(relayer, relayer); + } + function test_verify( uint32 origin, bytes32 sender, From c9bd7c3c52416f1c65cb413f33d422dfcc974d0f Mon Sep 17 00:00:00 2001 From: Kunal Arora <55632507+aroralanuk@users.noreply.github.com> Date: Tue, 22 Oct 2024 12:49:02 +0530 Subject: [PATCH 6/8] fix(contracts): `RateLimit` minor changes (#4575) ### Description - Added a check for invalid capacity and event for token level change ### Drive-by changes None ### Related issues - fixes https://github.com/chainlight-io/2024-08-hyperlane/issues/14 ### Backward compatibility Yes ### Testing Unit tests --- solidity/contracts/libs/RateLimited.sol | 42 +++++++++++++++++----- solidity/test/lib/RateLimited.t.sol | 47 ++++++++++++++++++++++++- 2 files changed, 80 insertions(+), 9 deletions(-) diff --git a/solidity/contracts/libs/RateLimited.sol b/solidity/contracts/libs/RateLimited.sol index 4dfb8b262..c58c60379 100644 --- a/solidity/contracts/libs/RateLimited.sol +++ b/solidity/contracts/libs/RateLimited.sol @@ -1,5 +1,19 @@ // SPDX-License-Identifier: MIT OR Apache-2.0 pragma solidity >=0.8.0; + +/*@@@@@@@ @@@@@@@@@ + @@@@@@@@@ @@@@@@@@@ + @@@@@@@@@ @@@@@@@@@ + @@@@@@@@@ @@@@@@@@@ + @@@@@@@@@@@@@@@@@@@@@@@@@ + @@@@@ HYPERLANE @@@@@@@ + @@@@@@@@@@@@@@@@@@@@@@@@@ + @@@@@@@@@ @@@@@@@@@ + @@@@@@@@@ @@@@@@@@@ + @@@@@@@@@ @@@@@@@@@ +@@@@@@@@@ @@@@@@@@*/ + +// ============ External Imports ============ import {OwnableUpgradeable} from "@openzeppelin/contracts-upgradeable/access/OwnableUpgradeable.sol"; /** @@ -7,16 +21,26 @@ import {OwnableUpgradeable} from "@openzeppelin/contracts-upgradeable/access/Own * @notice A contract used to keep track of an address sender's token amount limits. * @dev Implements a modified token bucket algorithm where the bucket is full in the beginning and gradually refills * See: https://dev.to/satrobit/rate-limiting-using-the-token-bucket-algorithm-3cjh - **/ + * + */ contract RateLimited is OwnableUpgradeable { uint256 public constant DURATION = 1 days; // 86400 - uint256 public filledLevel; /// @notice Current filled level - uint256 public refillRate; /// @notice Tokens per second refill rate - uint256 public lastUpdated; /// @notice Timestamp of the last time an action has been taken TODO prob can be uint40 + /// @notice Current filled level + uint256 public filledLevel; + /// @notice Tokens per second refill rate + uint256 public refillRate; + /// @notice Timestamp of the last time an action has been taken + uint256 public lastUpdated; event RateLimitSet(uint256 _oldCapacity, uint256 _newCapacity); + event ConsumedFilledLevel(uint256 filledLevel, uint256 lastUpdated); + constructor(uint256 _capacity) { + require( + _capacity >= DURATION, + "Capacity must be greater than DURATION" + ); _transferOwnership(msg.sender); setRefillRate(_capacity); filledLevel = _capacity; @@ -88,20 +112,22 @@ contract RateLimited is OwnableUpgradeable { /** * Validate an amount and decreases the currentCapacity - * @param _newAmount The amount to consume the fill level + * @param _consumedAmount The amount to consume the fill level * @return The new filled level */ function validateAndConsumeFilledLevel( - uint256 _newAmount + uint256 _consumedAmount ) public returns (uint256) { uint256 adjustedFilledLevel = calculateCurrentLevel(); - require(_newAmount <= adjustedFilledLevel, "RateLimitExceeded"); + require(_consumedAmount <= adjustedFilledLevel, "RateLimitExceeded"); // Reduce the filledLevel and update lastUpdated - uint256 _filledLevel = adjustedFilledLevel - _newAmount; + uint256 _filledLevel = adjustedFilledLevel - _consumedAmount; filledLevel = _filledLevel; lastUpdated = block.timestamp; + emit ConsumedFilledLevel(filledLevel, lastUpdated); + return _filledLevel; } } diff --git a/solidity/test/lib/RateLimited.t.sol b/solidity/test/lib/RateLimited.t.sol index 1276c86af..d0ea273de 100644 --- a/solidity/test/lib/RateLimited.t.sol +++ b/solidity/test/lib/RateLimited.t.sol @@ -1,5 +1,6 @@ // SPDX-License-Identifier: MIT or Apache-2.0 pragma solidity ^0.8.13; + import {Test} from "forge-std/Test.sol"; import {RateLimited} from "../../contracts/libs/RateLimited.sol"; @@ -13,8 +14,13 @@ contract RateLimitLibTest is Test { rateLimited = new RateLimited(MAX_CAPACITY); } + function testConstructor_revertsWhen_lowCapacity() public { + vm.expectRevert("Capacity must be greater than DURATION"); + new RateLimited(1 days - 1); + } + function testRateLimited_setsNewLimit() external { - rateLimited.setRefillRate(2 ether); + assert(rateLimited.setRefillRate(2 ether) > 0); assertApproxEqRel(rateLimited.maxCapacity(), 2 ether, ONE_PERCENT); assertEq(rateLimited.refillRate(), uint256(2 ether) / 1 days); // 2 ether / 1 day } @@ -45,6 +51,25 @@ contract RateLimitLibTest is Test { rateLimited.setRefillRate(1 ether); } + function testConsumedFilledLevelEvent() public { + uint256 consumeAmount = 0.5 ether; + + vm.expectEmit(true, true, false, true); + emit RateLimited.ConsumedFilledLevel( + 499999999999993600, + block.timestamp + ); // precision loss + rateLimited.validateAndConsumeFilledLevel(consumeAmount); + + assertApproxEqRelDecimal( + rateLimited.filledLevel(), + MAX_CAPACITY - consumeAmount, + 1e14, + 0 + ); + assertEq(rateLimited.lastUpdated(), block.timestamp); + } + function testRateLimited_neverReturnsGtMaxLimit( uint256 _newAmount, uint40 _newTime @@ -104,4 +129,24 @@ contract RateLimitLibTest is Test { currentTargetLimit = rateLimited.calculateCurrentLevel(); assertApproxEqRel(currentTargetLimit, MAX_CAPACITY, ONE_PERCENT); } + + function testCalculateCurrentLevel_revertsWhenCapacityIsZero() public { + rateLimited.setRefillRate(0); + + vm.expectRevert("RateLimitNotSet"); + rateLimited.calculateCurrentLevel(); + } + + function testValidateAndConsumeFilledLevel_revertsWhenExceedingLimit() + public + { + vm.warp(1 days); + uint256 initialLevel = rateLimited.calculateCurrentLevel(); + + uint256 excessAmount = initialLevel + 1 ether; + + vm.expectRevert("RateLimitExceeded"); + rateLimited.validateAndConsumeFilledLevel(excessAmount); + assertEq(rateLimited.calculateCurrentLevel(), initialLevel); + } } From 72c23c0d636dd9dc659542e129ea2f836255c50f Mon Sep 17 00:00:00 2001 From: Kunal Arora <55632507+aroralanuk@users.noreply.github.com> Date: Tue, 22 Oct 2024 14:23:22 +0530 Subject: [PATCH 7/8] fix(contracts): owner vault metadata compatibility (#4566) ### Description - Added PRECISION and rateUpdateNonce to ensure compatibility of HypERC4626 (even though this is not a useful warp route bc exchangeRate will stay at 1e10) ### Drive-by changes None ### Related issues - closes https://github.com/chainlight-io/2024-08-hyperlane/issues/9 ### Backward compatibility No ### Testing Unit test --- .changeset/sweet-humans-argue.md | 5 ++ .../contracts/token/extensions/HypERC4626.sol | 17 ++++++- .../token/extensions/HypERC4626Collateral.sol | 19 +++++++- .../extensions/HypERC4626OwnerCollateral.sol | 27 +++++++++-- .../HypERC20CollateralVaultDeposit.t.sol | 47 +++++++++++++++++++ 5 files changed, 110 insertions(+), 5 deletions(-) create mode 100644 .changeset/sweet-humans-argue.md diff --git a/.changeset/sweet-humans-argue.md b/.changeset/sweet-humans-argue.md new file mode 100644 index 000000000..3a6ff4647 --- /dev/null +++ b/.changeset/sweet-humans-argue.md @@ -0,0 +1,5 @@ +--- +'@hyperlane-xyz/core': minor +--- + +Added PRECISION and rateUpdateNonce to ensure compatibility of HypERC4626 diff --git a/solidity/contracts/token/extensions/HypERC4626.sol b/solidity/contracts/token/extensions/HypERC4626.sol index 141b081ba..9ceb5536b 100644 --- a/solidity/contracts/token/extensions/HypERC4626.sol +++ b/solidity/contracts/token/extensions/HypERC4626.sol @@ -1,13 +1,28 @@ // SPDX-License-Identifier: MIT OR Apache-2.0 pragma solidity >=0.8.0; +/*@@@@@@@ @@@@@@@@@ + @@@@@@@@@ @@@@@@@@@ + @@@@@@@@@ @@@@@@@@@ + @@@@@@@@@ @@@@@@@@@ + @@@@@@@@@@@@@@@@@@@@@@@@@ + @@@@@ HYPERLANE @@@@@@@ + @@@@@@@@@@@@@@@@@@@@@@@@@ + @@@@@@@@@ @@@@@@@@@ + @@@@@@@@@ @@@@@@@@@ + @@@@@@@@@ @@@@@@@@@ +@@@@@@@@@ @@@@@@@@*/ + +// ============ Internal Imports ============ import {IXERC20} from "../interfaces/IXERC20.sol"; import {HypERC20} from "../HypERC20.sol"; -import {Math} from "@openzeppelin/contracts/utils/math/Math.sol"; import {Message} from "../../libs/Message.sol"; import {TokenMessage} from "../libs/TokenMessage.sol"; import {TokenRouter} from "../libs/TokenRouter.sol"; +// ============ External Imports ============ +import {Math} from "@openzeppelin/contracts/utils/math/Math.sol"; + /** * @title Hyperlane ERC20 Rebasing Token * @author Abacus Works diff --git a/solidity/contracts/token/extensions/HypERC4626Collateral.sol b/solidity/contracts/token/extensions/HypERC4626Collateral.sol index 0ae32ab2f..87528b109 100644 --- a/solidity/contracts/token/extensions/HypERC4626Collateral.sol +++ b/solidity/contracts/token/extensions/HypERC4626Collateral.sol @@ -1,11 +1,26 @@ // SPDX-License-Identifier: Apache-2.0 pragma solidity >=0.8.0; -import "@openzeppelin/contracts/token/ERC20/extensions/ERC4626.sol"; +/*@@@@@@@ @@@@@@@@@ + @@@@@@@@@ @@@@@@@@@ + @@@@@@@@@ @@@@@@@@@ + @@@@@@@@@ @@@@@@@@@ + @@@@@@@@@@@@@@@@@@@@@@@@@ + @@@@@ HYPERLANE @@@@@@@ + @@@@@@@@@@@@@@@@@@@@@@@@@ + @@@@@@@@@ @@@@@@@@@ + @@@@@@@@@ @@@@@@@@@ + @@@@@@@@@ @@@@@@@@@ +@@@@@@@@@ @@@@@@@@*/ + +// ============ Internal Imports ============ import {TokenMessage} from "../libs/TokenMessage.sol"; import {HypERC20Collateral} from "../HypERC20Collateral.sol"; import {TypeCasts} from "../../libs/TypeCasts.sol"; +// ============ External Imports ============ +import "@openzeppelin/contracts/token/ERC20/extensions/ERC4626.sol"; + /** * @title Hyperlane ERC4626 Token Collateral with deposits collateral to a vault * @author Abacus Works @@ -17,7 +32,9 @@ contract HypERC4626Collateral is HypERC20Collateral { // Address of the ERC4626 compatible vault ERC4626 public immutable vault; + // Precision for the exchange rate uint256 public constant PRECISION = 1e10; + // Null recipient for rebase transfer bytes32 public constant NULL_RECIPIENT = 0x0000000000000000000000000000000000000000000000000000000000000001; // Nonce for the rate update, to ensure sequential updates diff --git a/solidity/contracts/token/extensions/HypERC4626OwnerCollateral.sol b/solidity/contracts/token/extensions/HypERC4626OwnerCollateral.sol index 1d4d64b0b..42d52f42c 100644 --- a/solidity/contracts/token/extensions/HypERC4626OwnerCollateral.sol +++ b/solidity/contracts/token/extensions/HypERC4626OwnerCollateral.sol @@ -1,9 +1,24 @@ // SPDX-License-Identifier: Apache-2.0 pragma solidity >=0.8.0; -import "@openzeppelin/contracts/token/ERC20/extensions/ERC4626.sol"; +/*@@@@@@@ @@@@@@@@@ + @@@@@@@@@ @@@@@@@@@ + @@@@@@@@@ @@@@@@@@@ + @@@@@@@@@ @@@@@@@@@ + @@@@@@@@@@@@@@@@@@@@@@@@@ + @@@@@ HYPERLANE @@@@@@@ + @@@@@@@@@@@@@@@@@@@@@@@@@ + @@@@@@@@@ @@@@@@@@@ + @@@@@@@@@ @@@@@@@@@ + @@@@@@@@@ @@@@@@@@@ +@@@@@@@@@ @@@@@@@@*/ + +// ============ Internal Imports ============ import {HypERC20Collateral} from "../HypERC20Collateral.sol"; +// ============ External Imports ============ +import {ERC4626} from "@openzeppelin/contracts/token/ERC20/extensions/ERC4626.sol"; + /** * @title Hyperlane ERC20 Token Collateral with deposits collateral to a vault, the yield goes to the owner * @author ltyu @@ -11,9 +26,12 @@ import {HypERC20Collateral} from "../HypERC20Collateral.sol"; contract HypERC4626OwnerCollateral is HypERC20Collateral { // Address of the ERC4626 compatible vault ERC4626 public immutable vault; - + // standby precision for exchange rate + uint256 public constant PRECISION = 1e10; // Internal balance of total asset deposited uint256 public assetDeposited; + // Nonce for the rate update, to ensure sequential updates (not necessary for Owner variant but for compatibility with HypERC4626) + uint32 public rateUpdateNonce; event ExcessSharesSwept(uint256 amount, uint256 assetsRedeemed); @@ -40,8 +58,11 @@ contract HypERC4626OwnerCollateral is HypERC20Collateral { function _transferFromSender( uint256 _amount ) internal override returns (bytes memory metadata) { - metadata = super._transferFromSender(_amount); + super._transferFromSender(_amount); _depositIntoVault(_amount); + rateUpdateNonce++; + + return abi.encode(PRECISION, rateUpdateNonce); } /** diff --git a/solidity/test/token/HypERC20CollateralVaultDeposit.t.sol b/solidity/test/token/HypERC20CollateralVaultDeposit.t.sol index 3dba941b3..8d2f9226e 100644 --- a/solidity/test/token/HypERC20CollateralVaultDeposit.t.sol +++ b/solidity/test/token/HypERC20CollateralVaultDeposit.t.sol @@ -16,8 +16,11 @@ pragma solidity ^0.8.13; import "forge-std/Test.sol"; import {TransparentUpgradeableProxy} from "@openzeppelin/contracts/proxy/transparent/TransparentUpgradeableProxy.sol"; +import {HypERC4626} from "../../contracts/token/extensions/HypERC4626.sol"; + import {ERC4626Test} from "../../contracts/test/ERC4626/ERC4626Test.sol"; import {TypeCasts} from "../../contracts/libs/TypeCasts.sol"; +import {TokenMessage} from "../../contracts/token/libs/TokenMessage.sol"; import {HypTokenTest} from "./HypERC20.t.sol"; import {HypERC4626OwnerCollateral} from "../../contracts/token/extensions/HypERC4626OwnerCollateral.sol"; @@ -227,6 +230,20 @@ contract HypERC4626OwnerCollateralTest is HypTokenTest { ); } + function testERC4626VaultDeposit_TransferFromSender_CorrectMetadata() + public + { + remoteToken = new HypERC4626(18, address(remoteMailbox), ORIGIN); + _enrollRemoteTokenRouter(); + vm.prank(ALICE); + + primaryToken.approve(address(localToken), TRANSFER_AMT); + _performRemoteTransfer(0, TRANSFER_AMT, 1); + + assertEq(HypERC4626(address(remoteToken)).exchangeRate(), 1e10); + assertEq(HypERC4626(address(remoteToken)).previousNonce(), 1); + } + function testBenchmark_overheadGasUsage() public override { vm.prank(ALICE); primaryToken.approve(address(localToken), TRANSFER_AMT); @@ -243,4 +260,34 @@ contract HypERC4626OwnerCollateralTest is HypTokenTest { uint256 gasAfter = gasleft(); console.log("Overhead gas usage: %d", gasBefore - gasAfter); } + + function _performRemoteTransfer( + uint256 _msgValue, + uint256 _amount, + uint32 _nonce + ) internal { + vm.prank(ALICE); + localToken.transferRemote{value: _msgValue}( + DESTINATION, + BOB.addressToBytes32(), + _amount + ); + + vm.expectEmit(true, true, false, true); + emit ReceivedTransferRemote(ORIGIN, BOB.addressToBytes32(), _amount); + bytes memory _tokenMessage = TokenMessage.format( + BOB.addressToBytes32(), + _amount, + abi.encode(uint256(1e10), _nonce) + ); + + vm.prank(address(remoteMailbox)); + remoteToken.handle( + ORIGIN, + address(localToken).addressToBytes32(), + _tokenMessage + ); + + assertEq(remoteToken.balanceOf(BOB), _amount); + } } From 013d3211c03bea0c554673f1e0969a205c7a865c Mon Sep 17 00:00:00 2001 From: Trevor Porter Date: Tue, 22 Oct 2024 12:13:14 +0100 Subject: [PATCH 8/8] fix: use hyp_message as tracing field instead of message (#4721) ### Description https://docs.rs/tracing/latest/tracing/#shorthand-macros > unlike other fields, `message`'s shorthand initialization is just the string itself. When using tracing::info etc, passing in an explicit `message` field takes precedence over the message string that's supplied as the second positional param! So we end up in situations where we see `message: HyperlaneMessage { id: ....` instead of the log message we want! e.g. https://cloudlogging.app.goo.gl/Nr8Q5W5KviD6sT8W9 Open to other field names, like `msg` or something, but this felt explicit ### Drive-by changes ### Related issues ### Backward compatibility ### Testing --- rust/main/agents/relayer/src/msg/gas_payment/mod.rs | 10 +++++----- .../agents/relayer/src/msg/metadata/multisig/base.rs | 4 ++-- .../src/msg/metadata/multisig/merkle_root_multisig.rs | 2 +- .../src/msg/metadata/multisig/message_id_multisig.rs | 2 +- rust/main/agents/relayer/src/msg/processor.rs | 2 +- .../chains/hyperlane-cosmos/src/mailbox/contract.rs | 2 +- rust/main/chains/hyperlane-fuel/src/mailbox.rs | 2 +- rust/main/hyperlane-base/src/db/rocks/hyperlane_db.rs | 4 ++-- 8 files changed, 14 insertions(+), 14 deletions(-) diff --git a/rust/main/agents/relayer/src/msg/gas_payment/mod.rs b/rust/main/agents/relayer/src/msg/gas_payment/mod.rs index a13d0d26e..fa9f40b45 100644 --- a/rust/main/agents/relayer/src/msg/gas_payment/mod.rs +++ b/rust/main/agents/relayer/src/msg/gas_payment/mod.rs @@ -105,7 +105,7 @@ impl GasPaymentEnforcer { for (policy, whitelist) in &self.policies { if !whitelist.msg_matches(message, true) { trace!( - msg=%message, + hyp_message=%message, ?policy, ?whitelist, "Message did not match whitelist for policy" @@ -114,13 +114,13 @@ impl GasPaymentEnforcer { } trace!( - msg=%message, + hyp_message=%message, ?policy, ?whitelist, "Message matched whitelist for policy" ); debug!( - msg=%message, + hyp_message=%message, ?policy, ?current_payment, ?current_expenditure, @@ -149,7 +149,7 @@ impl GasPaymentEnforcer { } error!( - msg=%message, + hyp_message=%message, policies=?self.policies, "No gas payment policy matched for message; consider adding a default policy to the end of the policies array which uses a wildcard whitelist." ); @@ -159,7 +159,7 @@ impl GasPaymentEnforcer { pub fn record_tx_outcome(&self, message: &HyperlaneMessage, outcome: TxOutcome) -> Result<()> { // This log is required in E2E, hence the use of a `const` debug!( - msg=%message, + hyp_message=%message, ?outcome, "{}", GAS_EXPENDITURE_LOG_MESSAGE, diff --git a/rust/main/agents/relayer/src/msg/metadata/multisig/base.rs b/rust/main/agents/relayer/src/msg/metadata/multisig/base.rs index 328b8848b..f7898c721 100644 --- a/rust/main/agents/relayer/src/msg/metadata/multisig/base.rs +++ b/rust/main/agents/relayer/src/msg/metadata/multisig/base.rs @@ -126,11 +126,11 @@ impl MetadataBuilder for T { .await .context(CTX)? { - debug!(?message, ?metadata.checkpoint, "Found checkpoint with quorum"); + debug!(hyp_message=?message, ?metadata.checkpoint, "Found checkpoint with quorum"); Ok(Some(self.format_metadata(metadata)?)) } else { info!( - ?message, ?validators, threshold, ism=%multisig_ism.address(), + hyp_message=?message, ?validators, threshold, ism=%multisig_ism.address(), "Could not fetch metadata: Unable to reach quorum" ); Ok(None) diff --git a/rust/main/agents/relayer/src/msg/metadata/multisig/merkle_root_multisig.rs b/rust/main/agents/relayer/src/msg/metadata/multisig/merkle_root_multisig.rs index b8bcca040..ea73c5537 100644 --- a/rust/main/agents/relayer/src/msg/metadata/multisig/merkle_root_multisig.rs +++ b/rust/main/agents/relayer/src/msg/metadata/multisig/merkle_root_multisig.rs @@ -45,7 +45,7 @@ impl MultisigIsmMetadataBuilder for MerkleRootMultisigMetadataBuilder { .await .context(CTX)?, debug!( - ?message, + hyp_message=?message, "No merkle leaf found for message id, must have not been enqueued in the tree" ) ); diff --git a/rust/main/agents/relayer/src/msg/metadata/multisig/message_id_multisig.rs b/rust/main/agents/relayer/src/msg/metadata/multisig/message_id_multisig.rs index 9866c98b0..ab920c326 100644 --- a/rust/main/agents/relayer/src/msg/metadata/multisig/message_id_multisig.rs +++ b/rust/main/agents/relayer/src/msg/metadata/multisig/message_id_multisig.rs @@ -42,7 +42,7 @@ impl MultisigIsmMetadataBuilder for MessageIdMultisigMetadataBuilder { .await .context(CTX)?, debug!( - ?message, + hyp_message=?message, "No merkle leaf found for message id, must have not been enqueued in the tree" ) ); diff --git a/rust/main/agents/relayer/src/msg/processor.rs b/rust/main/agents/relayer/src/msg/processor.rs index 43bd93c67..59ec32cb6 100644 --- a/rust/main/agents/relayer/src/msg/processor.rs +++ b/rust/main/agents/relayer/src/msg/processor.rs @@ -163,7 +163,7 @@ impl DirectionalNonceIterator { if let Some(message) = self.indexed_message_with_nonce()? { Self::update_max_nonce_gauge(&message, metrics); if !self.is_message_processed()? { - debug!(?message, iterator=?self, "Found processable message"); + debug!(hyp_message=?message, iterator=?self, "Found processable message"); return Ok(MessageStatus::Processable(message)); } else { return Ok(MessageStatus::Processed); diff --git a/rust/main/chains/hyperlane-cosmos/src/mailbox/contract.rs b/rust/main/chains/hyperlane-cosmos/src/mailbox/contract.rs index 7c3f1c4ee..9c793c93e 100644 --- a/rust/main/chains/hyperlane-cosmos/src/mailbox/contract.rs +++ b/rust/main/chains/hyperlane-cosmos/src/mailbox/contract.rs @@ -176,7 +176,7 @@ impl Mailbox for CosmosMailbox { Ok(tx_response_to_outcome(response)?) } - #[instrument(err, ret, skip(self), fields(msg=%message, metadata=%bytes_to_hex(metadata)))] + #[instrument(err, ret, skip(self), fields(hyp_message=%message, metadata=%bytes_to_hex(metadata)))] #[allow(clippy::blocks_in_conditions)] // TODO: `rustc` 1.80.1 clippy issue async fn process_estimate_costs( &self, diff --git a/rust/main/chains/hyperlane-fuel/src/mailbox.rs b/rust/main/chains/hyperlane-fuel/src/mailbox.rs index 3df6bdc9e..1c78e839e 100644 --- a/rust/main/chains/hyperlane-fuel/src/mailbox.rs +++ b/rust/main/chains/hyperlane-fuel/src/mailbox.rs @@ -185,7 +185,7 @@ impl Mailbox for FuelMailbox { } // Process cost of the `process` method - #[instrument(err, ret, skip(self), fields(msg=%message, metadata=%bytes_to_hex(metadata)))] + #[instrument(err, ret, skip(self), fields(hyp_message=%message, metadata=%bytes_to_hex(metadata)))] #[allow(clippy::blocks_in_conditions)] // TODO: `rustc` 1.80.1 clippy issue async fn process_estimate_costs( &self, diff --git a/rust/main/hyperlane-base/src/db/rocks/hyperlane_db.rs b/rust/main/hyperlane-base/src/db/rocks/hyperlane_db.rs index c99990b43..76a51bfe3 100644 --- a/rust/main/hyperlane-base/src/db/rocks/hyperlane_db.rs +++ b/rust/main/hyperlane-base/src/db/rocks/hyperlane_db.rs @@ -87,12 +87,12 @@ impl HyperlaneRocksDB { dispatched_block_number: u64, ) -> DbResult { if let Ok(Some(_)) = self.retrieve_message_id_by_nonce(&message.nonce) { - trace!(msg=?message, "Message already stored in db"); + trace!(hyp_message=?message, "Message already stored in db"); return Ok(false); } let id = message.id(); - debug!(msg=?message, "Storing new message in db",); + debug!(hyp_message=?message, "Storing new message in db",); // - `id` --> `message` self.store_message_by_id(&id, message)?;