Skip to content

Commit

Permalink
fix: prevent recovery with same setup
Browse files Browse the repository at this point in the history
  • Loading branch information
@iamacook
iamacook committed Nov 28, 2023
1 parent e9e0974 commit 52372d6
Show file tree
Hide file tree
Showing 4 changed files with 151 additions and 209 deletions.
89 changes: 71 additions & 18 deletions src/components/tx-flow/flows/RecoverAccount/RecoverAccountFlowSetup.tsx
View file
Original file line number Diff line number Diff line change
Expand Up @@ -9,9 +9,10 @@ import {
TextField,
IconButton,
Tooltip,
FormHelperText,
} from '@mui/material'
import { useForm, FormProvider, useFieldArray, Controller } from 'react-hook-form'
import { Fragment } from 'react'
import { Fragment, useCallback } from 'react'
import type { ReactElement } from 'react'

import TxCard from '../../common/TxCard'
Expand All @@ -24,17 +25,42 @@ import InfoIcon from '@/public/images/notifications/info.svg'
import useSafeInfo from '@/hooks/useSafeInfo'
import { sameAddress } from '@/utils/addresses'
import type { RecoverAccountFlowProps } from '.'
import type { AddressEx } from '@safe-global/safe-gateway-typescript-sdk'

import commonCss from '@/components/tx-flow/common/styles.module.css'

export function _isSameSetup({
oldOwners,
oldThreshold,
newOwners,
newThreshold,
}: {
oldOwners: Array<AddressEx>
oldThreshold: number
newOwners: Array<AddressEx>
newThreshold: number
}): boolean {
if (oldThreshold !== newThreshold) {
return false
}

if (oldOwners.length !== newOwners.length) {
return false
}

return oldOwners.every((oldOwner) => {
return newOwners.some((newOwner) => sameAddress(oldOwner.value, newOwner.value))
})
}

export function RecoverAccountFlowSetup({
params,
onSubmit,
}: {
params: RecoverAccountFlowProps
onSubmit: (formData: RecoverAccountFlowProps) => void
}): ReactElement {
const { safeAddress } = useSafeInfo()
const { safeAddress, safe } = useSafeInfo()

const formMethods = useForm<RecoverAccountFlowProps>({
defaultValues: params,
Expand All @@ -46,7 +72,12 @@ export function RecoverAccountFlowSetup({
name: RecoverAccountFlowFields.owners,
})

const owners = formMethods.watch(RecoverAccountFlowFields.owners)
const isSameSetup = useCallback(
(newOwners: Array<AddressEx>, newThreshold: number): boolean => {
return _isSameSetup({ oldOwners: safe.owners, oldThreshold: safe.threshold, newOwners, newThreshold })
},
[safe.owners, safe.threshold],
)

return (
<FormProvider {...formMethods}>
Expand Down Expand Up @@ -78,11 +109,18 @@ export function RecoverAccountFlowSetup({
return 'The Safe Account cannot own itself'
}

const isDuplicate = owners.filter((owner) => owner.value === value).length > 1
const newOwners = formMethods.getValues(RecoverAccountFlowFields.owners)
const isDuplicate = newOwners.filter((owner) => owner.value === value).length > 1
if (isDuplicate) {
return 'Already designated to be an owner'
}

const newThreshold = formMethods.getValues(RecoverAccountFlowFields.threshold)
if (isSameSetup(newOwners, Number(newThreshold))) {
return 'Proposed Account setup is the same'
}
}}
deps={[RecoverAccountFlowFields.threshold]}
/>
</Grid>

Expand Down Expand Up @@ -132,13 +170,13 @@ export function RecoverAccountFlowSetup({
</Typography>
</div>

<Grid container direction="row" alignItems="center" gap={2} mb={1}>
<Grid item>
<Controller
control={formMethods.control}
name={RecoverAccountFlowFields.threshold}
render={({ field }) => (
<TextField select {...field}>
<Controller
control={formMethods.control}
name={RecoverAccountFlowFields.threshold}
render={({ field, fieldState }) => (
<Grid container direction="row" alignItems="center" gap={2} mb={1}>
<Grid item>
<TextField select {...field} error={!!fieldState.error}>
{fields.map((_, index) => {
const value = index + 1
return (
Expand All @@ -148,14 +186,29 @@ export function RecoverAccountFlowSetup({
)
})}
</TextField>
)}
/>
</Grid>
</Grid>

<Grid item>
<Typography>out of {fields.length} owner(s)</Typography>
</Grid>
</Grid>
<Grid item>
<Typography>out of {fields.length} owner(s)</Typography>
</Grid>

{fieldState.error && (
<Grid item xs={12}>
<FormHelperText error>{fieldState.error?.message}</FormHelperText>
</Grid>
)}
</Grid>
)}
rules={{
validate: (newThreshold) => {
const newOwners = formMethods.getValues(RecoverAccountFlowFields.owners)
if (isSameSetup(newOwners, Number(newThreshold))) {
return 'Proposed Account setup is the same'
}
},
deps: [RecoverAccountFlowFields.owners],
}}
/>

<Divider className={commonCss.nestedDivider} />

Expand Down
78 changes: 78 additions & 0 deletions src/components/tx-flow/flows/RecoverAccount/__tests__/RecoverAccountFlowSetup.test.ts
View file
Original file line number Diff line number Diff line change
@@ -0,0 +1,78 @@
import { faker } from '@faker-js/faker'
import { shuffle } from 'lodash'
import type { AddressEx } from '@safe-global/safe-gateway-typescript-sdk'

import { _isSameSetup } from '../RecoverAccountFlowSetup'

describe('RecoverAccountFlowSetup', () => {
describe('isSameSetup', () => {
it('should return true if the owners and threshold are the same', () => {
const oldOwners: Array<AddressEx> = [
{ value: faker.finance.ethereumAddress() },
{ value: faker.finance.ethereumAddress() },
{ value: faker.finance.ethereumAddress() },
{ value: faker.finance.ethereumAddress() },
]
const oldThreshold = faker.number.int({ min: 1, max: oldOwners.length })

const newOwners = shuffle(oldOwners)

expect(
_isSameSetup({
oldOwners,
oldThreshold,
newOwners,
newThreshold: oldThreshold,
}),
).toBe(true)
})

it('should return false if the owners are the same but the threshold is different', () => {
const oldOwners: Array<AddressEx> = [
{ value: faker.finance.ethereumAddress() },
{ value: faker.finance.ethereumAddress() },
{ value: faker.finance.ethereumAddress() },
{ value: faker.finance.ethereumAddress() },
]
const oldThreshold = 1

const newOwners = shuffle(oldOwners)
const newThreshold = 2

expect(
_isSameSetup({
oldOwners,
oldThreshold,
newOwners,
newThreshold,
}),
).toBe(false)
})

it('should return false if the threshold is the same but the owners are different', () => {
const oldOwners: Array<AddressEx> = [
{ value: faker.finance.ethereumAddress() },
{ value: faker.finance.ethereumAddress() },
{ value: faker.finance.ethereumAddress() },
{ value: faker.finance.ethereumAddress() },
]
const oldThreshold = 2

const newOwners = [
{ value: faker.finance.ethereumAddress() },
{ value: faker.finance.ethereumAddress() },
{ value: faker.finance.ethereumAddress() },
{ value: faker.finance.ethereumAddress() },
]

expect(
_isSameSetup({
oldOwners,
oldThreshold,
newOwners,
newThreshold: oldThreshold,
}),
).toBe(false)
})
})
})
147 changes: 1 addition & 146 deletions src/services/recovery/__tests__/transaction.test.ts
View file
Original file line number Diff line number Diff line change
@@ -1,11 +1,9 @@
import { faker } from '@faker-js/faker'
import { Interface } from 'ethers/lib/utils'
import { SENTINEL_ADDRESS } from '@safe-global/safe-core-sdk/dist/src/utils/constants'
import { OperationType } from '@safe-global/safe-core-sdk-types'
import * as deployments from '@safe-global/safe-deployments'
import type { SafeInfo } from '@safe-global/safe-gateway-typescript-sdk'

import { getRecoveryProposalTransaction, getRecoveryProposalTransactions } from '../transaction'
import { getRecoveryProposalTransactions } from '../transaction'

describe('transaction', () => {
describe('getRecoveryTransactions', () => {
Expand Down Expand Up @@ -565,148 +563,5 @@ describe('transaction', () => {
})
})
})

it('should throw if the new threshold is higher than the final owner output', () => {
const safeAddresss = faker.finance.ethereumAddress()

const newOwner1 = faker.finance.ethereumAddress()
const newOwner2 = faker.finance.ethereumAddress()

const oldOwner1 = faker.finance.ethereumAddress()

const oldThreshold = 1
const newThreshold = 10

const safe = {
address: { value: safeAddresss },
owners: [{ value: oldOwner1 }],
threshold: oldThreshold,
} as SafeInfo

const newOwners = [{ value: newOwner1 }, { value: newOwner2 }]

expect(() => getRecoveryProposalTransactions({ safe, newThreshold, newOwners })).toThrow(
'New threshold is higher than desired owners',
)
})
})

describe('getRecoveryProposalTransaction', () => {
it('should throw an error when no recovery transactions are found', () => {
const safe = {
address: { value: faker.finance.ethereumAddress() },
owners: [{ value: faker.finance.ethereumAddress() }],
threshold: 1,
} as SafeInfo

expect(() =>
getRecoveryProposalTransaction({
safe,
newThreshold: safe.threshold,
newOwners: safe.owners,
}),
).toThrow('No recovery transactions found')
})

it('should return the transaction when a single recovery transaction is found', () => {
const safeAddresss = faker.finance.ethereumAddress()

const oldOwner1 = faker.finance.ethereumAddress()
const newOwner1 = faker.finance.ethereumAddress()

const oldThreshold = 1

const safe = {
address: { value: safeAddresss },
owners: [{ value: oldOwner1 }],
threshold: oldThreshold,
} as SafeInfo

const newOwners = [{ value: newOwner1 }]

const transaction = getRecoveryProposalTransaction({
safe,
newThreshold: oldThreshold,
newOwners,
})

expect(transaction).toEqual({
to: safeAddresss,
value: '0',
data: expect.any(String),
operation: OperationType.Call,
})
})

describe('when multiple recovery transactions are found', () => {
it('should return a MetaTransactionData object ', () => {
const safeAddresss = faker.finance.ethereumAddress()

const oldOwner1 = faker.finance.ethereumAddress()
const oldOwner2 = faker.finance.ethereumAddress()
const oldOwner3 = faker.finance.ethereumAddress()

const newOwner1 = faker.finance.ethereumAddress()
const newOwner2 = faker.finance.ethereumAddress()
const newOwner3 = faker.finance.ethereumAddress()

const oldThreshold = 2
const newThreshold = oldThreshold + 1

const safe = {
address: { value: safeAddresss },
owners: [{ value: oldOwner1 }, { value: oldOwner2 }, { value: oldOwner3 }],
threshold: oldThreshold,
} as SafeInfo

const multiSendDeployment = deployments.getMultiSendCallOnlyDeployment()!

const newOwners = [{ value: newOwner1 }, { value: newOwner2 }, { value: newOwner3 }]

const transaction = getRecoveryProposalTransaction({
safe,
newThreshold,
newOwners,
})

expect(transaction).toEqual({
to: multiSendDeployment.defaultAddress,
value: '0',
data: expect.any(String),
operation: OperationType.Call,
})
})

it('should throw an error when MultiSend deployment is not found', () => {
jest.spyOn(deployments, 'getMultiSendCallOnlyDeployment').mockReturnValue(undefined)

const safeAddresss = faker.finance.ethereumAddress()

const oldOwner1 = faker.finance.ethereumAddress()
const oldOwner2 = faker.finance.ethereumAddress()

const newOwner1 = faker.finance.ethereumAddress()
const newOwner2 = faker.finance.ethereumAddress()
const newOwner3 = faker.finance.ethereumAddress()

const oldThreshold = 2

const safe = {
address: { value: safeAddresss },
owners: [{ value: oldOwner1 }, { value: oldOwner2 }],
threshold: oldThreshold,
} as SafeInfo

const newOwners = [{ value: newOwner1 }, { value: newOwner2 }, { value: newOwner3 }]

expect(() =>
getRecoveryProposalTransaction({
safe,
newThreshold: oldThreshold,
newOwners,
}),
).toThrow('MultiSend deployment not found')
})
})
})
})
Loading

0 comments on commit 52372d6

Please sign in to comment.