diff --git a/src/services/recovery/__tests__/transaction.test.ts b/src/services/recovery/__tests__/transaction.test.ts index faf447c8d8..a3d5b56ed9 100644 --- a/src/services/recovery/__tests__/transaction.test.ts +++ b/src/services/recovery/__tests__/transaction.test.ts @@ -78,8 +78,8 @@ describe('transaction', () => { oldOwner1, newOwner1, ]) - expect(encodeFunctionDataSpy).toHaveBeenNthCalledWith(2, 'swapOwner', [oldOwner1, oldOwner2, newOwner2]) - expect(encodeFunctionDataSpy).toHaveBeenNthCalledWith(3, 'swapOwner', [oldOwner2, oldOwner3, newOwner3]) + expect(encodeFunctionDataSpy).toHaveBeenNthCalledWith(2, 'swapOwner', [newOwner1, oldOwner2, newOwner2]) + expect(encodeFunctionDataSpy).toHaveBeenNthCalledWith(3, 'swapOwner', [newOwner2, oldOwner3, newOwner3]) }) }) @@ -115,8 +115,8 @@ describe('transaction', () => { oldOwner1, newOwner1, ]) - expect(encodeFunctionDataSpy).toHaveBeenNthCalledWith(2, 'swapOwner', [oldOwner1, oldOwner2, newOwner2]) - expect(encodeFunctionDataSpy).toHaveBeenNthCalledWith(3, 'swapOwner', [oldOwner2, oldOwner3, newOwner3]) + expect(encodeFunctionDataSpy).toHaveBeenNthCalledWith(2, 'swapOwner', [newOwner1, oldOwner2, newOwner2]) + expect(encodeFunctionDataSpy).toHaveBeenNthCalledWith(3, 'swapOwner', [newOwner2, oldOwner3, newOwner3]) expect(encodeFunctionDataSpy).toHaveBeenNthCalledWith(4, 'changeThreshold', [newThreshold]) }) }) @@ -176,7 +176,7 @@ describe('transaction', () => { oldOwner1, newOwner1, ]) - expect(encodeFunctionDataSpy).toHaveBeenNthCalledWith(2, 'swapOwner', [oldOwner1, oldOwner2, newOwner2]) + expect(encodeFunctionDataSpy).toHaveBeenNthCalledWith(2, 'swapOwner', [newOwner1, oldOwner2, newOwner2]) }) }) @@ -211,7 +211,7 @@ describe('transaction', () => { oldOwner1, newOwner1, ]) - expect(encodeFunctionDataSpy).toHaveBeenNthCalledWith(2, 'swapOwner', [oldOwner1, oldOwner2, newOwner2]) + expect(encodeFunctionDataSpy).toHaveBeenNthCalledWith(2, 'swapOwner', [newOwner1, oldOwner2, newOwner2]) expect(encodeFunctionDataSpy).toHaveBeenNthCalledWith(3, 'changeThreshold', [newThreshold]) }) }) @@ -309,7 +309,7 @@ describe('transaction', () => { oldOwner1, newOwner1, ]) - expect(encodeFunctionDataSpy).toHaveBeenNthCalledWith(2, 'swapOwner', [oldOwner1, oldOwner2, newOwner2]) + expect(encodeFunctionDataSpy).toHaveBeenNthCalledWith(2, 'swapOwner', [newOwner1, oldOwner2, newOwner2]) expect(encodeFunctionDataSpy).toHaveBeenNthCalledWith(3, 'addOwnerWithThreshold', [newOwner3, oldThreshold]) }) }) @@ -345,7 +345,7 @@ describe('transaction', () => { oldOwner1, newOwner1, ]) - expect(encodeFunctionDataSpy).toHaveBeenNthCalledWith(2, 'swapOwner', [oldOwner1, oldOwner2, newOwner2]) + expect(encodeFunctionDataSpy).toHaveBeenNthCalledWith(2, 'swapOwner', [newOwner1, oldOwner2, newOwner2]) expect(encodeFunctionDataSpy).toHaveBeenNthCalledWith(3, 'addOwnerWithThreshold', [newOwner3, newThreshold]) }) }) @@ -374,20 +374,23 @@ describe('transaction', () => { expect(transactions).toHaveLength(2) expect(encodeFunctionDataSpy).toHaveBeenCalledTimes(2) - expect(encodeFunctionDataSpy).toHaveBeenNthCalledWith(1, 'addOwnerWithThreshold', [newOwner2, 1]) + expect(encodeFunctionDataSpy).toHaveBeenNthCalledWith(1, 'addOwnerWithThreshold', [newOwner2, oldThreshold]) expect(encodeFunctionDataSpy).toHaveBeenNthCalledWith(2, 'addOwnerWithThreshold', [newOwner3, oldThreshold]) }) it('should swap as many differing owners as possible then add the rest when with a final threshold change if the threshold changes', () => { const safeAddresss = faker.finance.ethereumAddress() - const oldOwner1 = faker.finance.ethereumAddress() - const newOwner1 = faker.finance.ethereumAddress() const newOwner2 = faker.finance.ethereumAddress() + const newOwner3 = faker.finance.ethereumAddress() + const newOwner4 = faker.finance.ethereumAddress() + const newOwner5 = faker.finance.ethereumAddress() - const oldThreshold = 2 - const newThreshold = oldThreshold + 1 + const oldOwner1 = faker.finance.ethereumAddress() + + const oldThreshold = 1 + const newThreshold = 4 const safe = { address: { value: safeAddresss }, @@ -395,15 +398,34 @@ describe('transaction', () => { threshold: oldThreshold, } as SafeInfo - const newOwners = [{ value: oldOwner1 }, { value: newOwner1 }, { value: newOwner2 }] + const newOwners = [ + { value: newOwner1 }, + { value: newOwner2 }, + { value: newOwner3 }, + { value: newOwner4 }, + { value: newOwner5 }, + ] const transactions = getRecoveryProposalTransactions({ safe, newThreshold, newOwners }) - expect(transactions).toHaveLength(2) + expect(transactions).toHaveLength(5) - expect(encodeFunctionDataSpy).toHaveBeenCalledTimes(2) - expect(encodeFunctionDataSpy).toHaveBeenNthCalledWith(1, 'addOwnerWithThreshold', [newOwner1, 1]) - expect(encodeFunctionDataSpy).toHaveBeenNthCalledWith(2, 'addOwnerWithThreshold', [newOwner2, newThreshold]) + expect(encodeFunctionDataSpy).toHaveBeenCalledTimes(5) + expect(encodeFunctionDataSpy).toHaveBeenNthCalledWith(1, 'swapOwner', [ + SENTINEL_ADDRESS, + oldOwner1, + newOwner1, + ]) + expect(encodeFunctionDataSpy).toHaveBeenNthCalledWith(2, 'addOwnerWithThreshold', [ + newOwner2, + 2, // Intemediary threshold - length of current owners + ]) + expect(encodeFunctionDataSpy).toHaveBeenNthCalledWith(3, 'addOwnerWithThreshold', [ + newOwner3, + 3, // Intemediary threshold - length of current owners + ]) + expect(encodeFunctionDataSpy).toHaveBeenNthCalledWith(4, 'addOwnerWithThreshold', [newOwner4, newThreshold]) + expect(encodeFunctionDataSpy).toHaveBeenNthCalledWith(5, 'addOwnerWithThreshold', [newOwner5, newThreshold]) }) }) }) @@ -444,8 +466,8 @@ describe('transaction', () => { oldOwner1, newOwner1, ]) - expect(encodeFunctionDataSpy).toHaveBeenNthCalledWith(2, 'swapOwner', [oldOwner1, oldOwner2, newOwner2]) - expect(encodeFunctionDataSpy).toHaveBeenNthCalledWith(3, 'removeOwner', [oldOwner2, oldOwner3, oldThreshold]) + expect(encodeFunctionDataSpy).toHaveBeenNthCalledWith(2, 'swapOwner', [newOwner1, oldOwner2, newOwner2]) + expect(encodeFunctionDataSpy).toHaveBeenNthCalledWith(3, 'removeOwner', [newOwner2, oldOwner3, oldThreshold]) }) it('should swap as many owners as possible then remove the rest when with a final threshold change if the threshold changes', () => { @@ -479,8 +501,8 @@ describe('transaction', () => { oldOwner1, newOwner1, ]) - expect(encodeFunctionDataSpy).toHaveBeenNthCalledWith(2, 'swapOwner', [oldOwner1, oldOwner2, newOwner2]) - expect(encodeFunctionDataSpy).toHaveBeenNthCalledWith(3, 'removeOwner', [oldOwner2, oldOwner3, newThreshold]) + expect(encodeFunctionDataSpy).toHaveBeenNthCalledWith(2, 'swapOwner', [newOwner1, oldOwner2, newOwner2]) + expect(encodeFunctionDataSpy).toHaveBeenNthCalledWith(3, 'removeOwner', [newOwner2, oldOwner3, newThreshold]) }) }) @@ -510,7 +532,7 @@ describe('transaction', () => { expect(encodeFunctionDataSpy).toHaveBeenCalledTimes(2) expect(encodeFunctionDataSpy).toHaveBeenNthCalledWith(1, 'swapOwner', [oldOwner1, oldOwner2, newOwner1]) - expect(encodeFunctionDataSpy).toHaveBeenNthCalledWith(2, 'removeOwner', [oldOwner2, oldOwner3, oldThreshold]) + expect(encodeFunctionDataSpy).toHaveBeenNthCalledWith(2, 'removeOwner', [newOwner1, oldOwner3, oldThreshold]) }) it('should swap as many differing owners as possible then remove the rest when with a final threshold change if the threshold changes', () => { @@ -539,10 +561,34 @@ describe('transaction', () => { expect(encodeFunctionDataSpy).toHaveBeenCalledTimes(2) expect(encodeFunctionDataSpy).toHaveBeenNthCalledWith(1, 'swapOwner', [oldOwner1, oldOwner2, newOwner1]) - expect(encodeFunctionDataSpy).toHaveBeenNthCalledWith(2, 'removeOwner', [oldOwner2, oldOwner3, newThreshold]) + expect(encodeFunctionDataSpy).toHaveBeenNthCalledWith(2, 'removeOwner', [newOwner1, oldOwner3, newThreshold]) }) }) }) + + 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', () => { diff --git a/src/services/recovery/transaction.ts b/src/services/recovery/transaction.ts index 9cbe01b4bc..dd36048bad 100644 --- a/src/services/recovery/transaction.ts +++ b/src/services/recovery/transaction.ts @@ -16,8 +16,6 @@ export function getRecoveryProposalTransactions({ newThreshold: number newOwners: Array }): Array { - const INTERMEDIARY_THRESHOLD = 1 - const safeDeployment = getSafeSingletonDeployment({ network: safe.chainId, version: safe.version ?? undefined }) if (!safeDeployment) { @@ -40,40 +38,70 @@ export function getRecoveryProposalTransactions({ // Check whether threshold should be changed after owner management let changeThreshold = newThreshold !== safe.threshold + // Owner management transaction data const txData: Array = [] + // Minimum number of owner management transactions required const length = Math.max(ownersToAdd.length, ownersToRemove.length) + // Iterate of existing/new owners and swap, add, remove accordingly for (let index = 0; index < length; index++) { const ownerToAdd = ownersToAdd[index]?.value const ownerToRemove = ownersToRemove[index]?.value - const ownerIndex = _owners.findIndex((owner) => sameAddress(owner.value, ownerToRemove)) - const prevOwner = ownerIndex === 0 ? SENTINEL_ADDRESS : _owners[ownerIndex - 1].value + const prevOwner = (() => { + const ownerIndex = _owners.findIndex((owner) => sameAddress(owner.value, ownerToRemove)) + return ownerIndex === 0 ? SENTINEL_ADDRESS : _owners[ownerIndex - 1]?.value + })() - // Swap owner if possible + // Swap existing owner with new one if (ownerToRemove && ownerToAdd) { - txData.push(safeInterface.encodeFunctionData('swapOwner', [prevOwner, ownerToRemove, ownerToAdd])) + const swapOwner = safeInterface.encodeFunctionData('swapOwner', [prevOwner, ownerToRemove, ownerToAdd]) + txData.push(swapOwner) + + // Swap owner in cache _owners = _owners.map((owner) => (sameAddress(owner.value, ownerToRemove) ? ownersToAdd[index] : owner)) continue } - // Add or remove owner, finally setting threshold (intermediary value prevents threshold > owner length) - const threshold = index === length - 1 ? newThreshold : INTERMEDIARY_THRESHOLD + const threshold = (() => { + const newOwnerLength = ownerToAdd ? _owners.length + 1 : _owners.length - 1 + + // Final transaction + if (index === length - 1) { + if (newThreshold > newOwnerLength) { + throw new Error('New threshold is higher than desired owners') + } + return newThreshold + } + + // Prevent intermediary threshold > number of owners + return newThreshold > newOwnerLength ? newOwnerLength : newThreshold + })() + + // Add new owner and set threshold + if (ownerToAdd) { + const addOwnerWithThreshold = safeInterface.encodeFunctionData('addOwnerWithThreshold', [ownerToAdd, threshold]) + txData.push(addOwnerWithThreshold) + + // Add owner to cache + _owners.push({ value: ownerToAdd }) + } + // Remove existing owner and set threshold + else { + const removeOwner = safeInterface.encodeFunctionData('removeOwner', [prevOwner, ownerToRemove, threshold]) + txData.push(removeOwner) - if (!ownerToRemove) { - txData.push(safeInterface.encodeFunctionData('addOwnerWithThreshold', [ownerToAdd, threshold])) - _owners.push(ownersToAdd[index]) - } else { - txData.push(safeInterface.encodeFunctionData('removeOwner', [prevOwner, ownerToRemove, threshold])) + // Remove owner from cache _owners = _owners.filter((owner) => !sameAddress(owner.value, ownerToRemove)) } + // addOwnerWithThreshold/removeOwner changed threshold changeThreshold = false } - // If only swapOwners exist but there is a threshold change, changeThreshold + // Only swapOwner will be called if (changeThreshold) { txData.push(safeInterface.encodeFunctionData('changeThreshold', [newThreshold])) }