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

feat: multisend contract detection #58

Merged
merged 9 commits into from
Jun 12, 2024

Conversation

banteg
Copy link
Contributor

@banteg banteg commented Jun 12, 2024

What I did

instead of hardcoding the addresses of multisend contracts on different networks, one can observe there are ever only two addresses, with pre-signed deployment transactions available for both.

instead, we just check the code of all addresses and use the first one that matches the expected code.

i also switched MultiSend for MultiSendCallOnly contract as it doesn't allow making delegatecalls that can modify the safe storage, potentially bricking it or stealing all funds.

both brownie-safe and safe web ui have switched to MultiSendCallOnly as the default.

we only ever delegatecall to the multisend contract itself, which we know is stateless and has the code we matched against the hardcoded bytecode.

fixes: #51

How I did it

How to verify it

from ape_safe.multisend import MultiSend

txn = MultiSend()
print(txn.contract)  # should detect the contract automatically

Checklist

  • Passes all linting checks (pre-commit and CI jobs)
  • New test cases have been added and are passing
  • Documentation has been updated
  • PR title follows Conventional Commit standard (will be automatically included in the changelog)

Copy link
Member

@fubuloubu fubuloubu left a comment

Choose a reason for hiding this comment

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

btw we can probably refactor to using a manifest bundled with the package a la ApeWorX/uniswap-sdk@bd6b26e

Copy link
Member

@fubuloubu fubuloubu left a comment

Choose a reason for hiding this comment

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

Maybe manifests/ instead of data/

@fubuloubu fubuloubu enabled auto-merge (squash) June 12, 2024 22:55
auto-merge was automatically disabled June 12, 2024 23:04

Head branch was pushed to by a user without write access

@fubuloubu fubuloubu enabled auto-merge (squash) June 12, 2024 23:08
@fubuloubu fubuloubu merged commit b4e7943 into ApeWorX:main Jun 12, 2024
13 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

simplify multisend contract address detection
2 participants