Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix(Multichain): validate account data #4302

Merged
merged 1 commit into from
Oct 7, 2024
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Original file line number Diff line number Diff line change
Expand Up @@ -76,6 +76,57 @@ describe('useSafeCreationData', () => {
})
})

it('should return undefined without chain info', async () => {
const safeAddress = faker.finance.ethereumAddress()
const chainInfos: ChainInfo[] = []
const { result } = renderHook(() => useSafeCreationData(safeAddress, chainInfos))
await waitFor(async () => {
await Promise.resolve()
expect(result.current).toEqual([undefined, undefined, false])
})
})

it('should throw an error for replayed Safe it uses a unknown to address', async () => {
const safeAddress = faker.finance.ethereumAddress()
const chainInfos = [chainBuilder().with({ chainId: '1' }).build()]
const undeployedSafe: UndeployedSafe = {
props: {
factoryAddress: faker.finance.ethereumAddress(),
saltNonce: '420',
masterCopy: faker.finance.ethereumAddress(),
safeVersion: '1.3.0',
safeAccountConfig: {
owners: [faker.finance.ethereumAddress(), faker.finance.ethereumAddress()],
threshold: 1,
data: faker.string.hexadecimal({ length: 64 }),
to: faker.finance.ethereumAddress(),
fallbackHandler: faker.finance.ethereumAddress(),
payment: 0,
paymentToken: ZERO_ADDRESS,
paymentReceiver: ZERO_ADDRESS,
},
},
status: {
status: PendingSafeStatus.AWAITING_EXECUTION,
type: PayMethod.PayLater,
},
}

const { result } = renderHook(() => useSafeCreationData(safeAddress, chainInfos), {
initialReduxState: {
undeployedSafes: {
'1': {
[safeAddress]: undeployedSafe,
},
},
},
})
await waitFor(async () => {
await Promise.resolve()
expect(result.current).toEqual([undefined, new Error(SAFE_CREATION_DATA_ERRORS.UNKNOWN_SETUP_MODULES), false])
})
})

it('should throw an error for legacy counterfactual Safes', async () => {
const safeAddress = faker.finance.ethereumAddress()
const chainInfos = [chainBuilder().with({ chainId: '1', l2: false }).build()]
Expand Down
30 changes: 19 additions & 11 deletions src/features/multichain/hooks/useSafeCreationData.ts
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Out of scope here but I wonder if we need this nested try catch structure inside useSafeCreationData. If there is an error for one of the chains, shouldn't it already propagate to the outer catch where we log the _816 error?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The idea is to retry it with other chains if a Safe is deployed on other chains already. Gnosis Chain does not work in most cases for instance.

Original file line number Diff line number Diff line change
Expand Up @@ -13,6 +13,7 @@ import { asError } from '@/services/exceptions/utils'
import semverSatisfies from 'semver/functions/satisfies'
import { ZERO_ADDRESS } from '@safe-global/protocol-kit/dist/src/utils/constants'
import { getSafeToL2SetupDeployment } from '@safe-global/safe-deployments'
import { type SafeAccountConfig } from '@safe-global/protocol-kit'

export const SAFE_CREATION_DATA_ERRORS = {
TX_NOT_FOUND: 'The Safe creation transaction could not be found. Please retry later.',
Expand Down Expand Up @@ -51,6 +52,19 @@ const getUndeployedSafeCreationData = async (undeployedSafe: UndeployedSafe): Pr
return undeployedSafe.props
}

const validateAccountConfig = (safeAccountConfig: SafeAccountConfig) => {
// Safes that used the reimbursement logic are not supported
if ((safeAccountConfig.payment && safeAccountConfig.payment > 0) || safeAccountConfig.paymentToken !== ZERO_ADDRESS) {
throw new Error(SAFE_CREATION_DATA_ERRORS.PAYMENT_SAFE)
}

const setupToL2Address = getSafeToL2SetupDeployment({ version: '1.4.1' })?.defaultAddress
if (safeAccountConfig.to !== ZERO_ADDRESS && !sameAddress(safeAccountConfig.to, setupToL2Address)) {
// Unknown setupModules calls cannot be replayed as the target contract is likely not deployed across chains
throw new Error(SAFE_CREATION_DATA_ERRORS.UNKNOWN_SETUP_MODULES)
}
}

const proxyFactoryInterface = Safe_proxy_factory__factory.createInterface()
const createProxySelector = proxyFactoryInterface.getFunction('createProxyWithNonce').selector

Expand All @@ -68,7 +82,10 @@ const getCreationDataForChain = async (
): Promise<ReplayedSafeProps> => {
// 1. The safe is counterfactual
if (undeployedSafe) {
return getUndeployedSafeCreationData(undeployedSafe)
const undeployedCreationData = await getUndeployedSafeCreationData(undeployedSafe)
validateAccountConfig(undeployedCreationData.safeAccountConfig)

return undeployedCreationData
}

const { data: creation } = await getCreationTransaction({
Expand All @@ -90,16 +107,7 @@ const getCreationDataForChain = async (

const safeAccountConfig = decodeSetupData(creation.setupData)

// Safes that used the reimbursement logic are not supported
if ((safeAccountConfig.payment && safeAccountConfig.payment > 0) || safeAccountConfig.paymentToken !== ZERO_ADDRESS) {
throw new Error(SAFE_CREATION_DATA_ERRORS.PAYMENT_SAFE)
}

const setupToL2Address = getSafeToL2SetupDeployment({ version: '1.4.1' })?.defaultAddress
if (safeAccountConfig.to !== ZERO_ADDRESS && !sameAddress(safeAccountConfig.to, setupToL2Address)) {
// Unknown setupModules calls cannot be replayed as the target contract is likely not deployed across chains
throw new Error(SAFE_CREATION_DATA_ERRORS.UNKNOWN_SETUP_MODULES)
}
validateAccountConfig(safeAccountConfig)

// We need to create a readOnly provider of the deployed chain
const customRpcUrl = chain ? customRpc?.[chain.chainId] : undefined
Expand Down
Loading