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: multisend verification only worked for the latest contract version #2439

Merged
merged 6 commits into from
Aug 29, 2023

Conversation

katspaugh
Copy link
Member

@katspaugh katspaugh commented Aug 24, 2023

What it solves

safe-deployments 1.2.6 made Safe 1.4.1 the default. See safe-global/safe-deployments#248

This breaks our code because we were always retrieving multisend contract addresses w/o a version. It worked for 1.3.0 but not for any other version.

How to test

  • Open a single transaction view for a multisend tx on Safe 1.3.0 on mainnet
  • Open a single transaction view for a multisend tx on Safe 1.1.1 on mainnet
  • Open a single transaction view for a multisend tx on Safe 1.3.0 on Polygon
  • Multisend details should be displayed for all of them
  • Create and execute a multisend tx in a 1.3.0 Safe
  • Create and execute a multisend tx in a 1.1.1 Safe

@github-actions
Copy link

github-actions bot commented Aug 24, 2023

Branch preview

✅ Deploy successful!

https://fix_multisend--walletweb.review-wallet-web.5afe.dev

@github-actions
Copy link

github-actions bot commented Aug 24, 2023

ESLint Summary View Full Report

Annotations are provided inline on the Files Changed tab. You can also see all annotations that were generated on the annotations page.

Type Occurrences Fixable
Errors 0 0
Warnings 0 0
Ignored 0 N/A
  • Result: ✅ success
  • Annotations: 0 total

Report generated by eslint-plus-action

src/utils/transaction-guards.ts Outdated Show resolved Hide resolved
@katspaugh
Copy link
Member Author

katspaugh commented Aug 25, 2023

I've realized this is not a good solution either because if a Safe Account is upgraded, existing transactions from a previous version won't be detected as multisend correctly.

Perhaps we should check the isTrusted flag instead of comparing contract addresses. Richard said this flag is not a good indicator of a multisend contract, it just tells us if a contract is "trusted" for delegate calls.

@iamacook
Copy link
Member

I've realized this is not a good solution either because if a Safe Account is upgraded, existing transactions from a previous version won't be detected as multisend correctly.

Perhaps we should check the isTrusted flag instead of comparing contract addresses.

We could check whether it's supported by looping over an array of contract versions. We could get the array from either the SafeVersions in the SDK, or the released assets of the deployments.

@katspaugh katspaugh changed the title Fix: pass safe version to multisend contract getters Fix: multisend verification only worked for the latest contract version; use trustedForDelegateCall instead Aug 25, 2023
@iamacook iamacook self-requested a review August 25, 2023 07:28
@katspaugh katspaugh changed the title Fix: multisend verification only worked for the latest contract version; use trustedForDelegateCall instead Fix: multisend verification only worked for the latest contract version Aug 25, 2023
@katspaugh
Copy link
Member Author

@iamacook yeah, I was thinking the same but after talking with infra, we should probably do it on CGW, see safe-global/safe-client-gateway#657.

So I suggest we just check the current Safe version at this time, just for safe-deployments 1.2.6 to still work, and do a more robust solution on CGW.

@katspaugh katspaugh added the bug Something isn't working label Aug 25, 2023
@katspaugh
Copy link
Member Author

Looks like this verification was added not for security reasons but just to not break the UI when some other contract has a multiSend method. See 5afe/safe-react#4023

I've removed the isSupportedMultisendContract function and enhanced isMultiSendTxInfo instead. The visual component that renders multisend actions also has guards inside that would just return early if no nested actions are found.

@@ -132,7 +109,7 @@ export const getMultiSendCallOnlyContract = (
const ethAdapter = createEthersAdapter(provider)

return ethAdapter.getMultiSendCallOnlyContract({
singletonDeployment: getMultiSendCallOnlyContractDeployment(chainId),
singletonDeployment: getMultiSendCallOnlyDeployment({ network: chainId, version: safeVersion || undefined }),
Copy link
Member Author

Choose a reason for hiding this comment

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

This is also an important fix, because it was getting 1.3.0 multisend by default which would break any time.

@francovenica
Copy link
Contributor

To do a multisend I just batched 2 send funds tx in a single tx, made sure that interacts with 0x40A2aCCbd92BCA938b02010E17A5b8929b49130D since that's the multisig contract

I compared prod with this PR and the tx looks the same, but still nothing out of order there so is ok
I was able to successfully create and execute tx in safes 1.1.1 and 1.3.0 in ETH and 1.3.0 in Poly

Looks good to me

@katspaugh katspaugh merged commit 9134da3 into dev Aug 29, 2023
9 checks passed
@katspaugh katspaugh deleted the fix-multisend branch August 29, 2023 05:53
@github-actions github-actions bot locked and limited conversation to collaborators Aug 29, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
bug Something isn't working
Projects
Archived in project
Development

Successfully merging this pull request may close these issues.

3 participants