safe-global / safe-smart-account

Safe allows secure management of blockchain assets.
https://safe.global
GNU Lesser General Public License v3.0
1.84k stars 907 forks source link

Add ability to implicitly specify "self" in multi-send #695

Closed Philogy closed 10 months ago

Philogy commented 10 months ago

Safes may want to be able to use multi-send for combining configuring calls such as enableModule, setGuard with each other or other external calls however specifying the safe's own address is impossible to currently do in initialization calls as the create2 salt for the safe depends on the hash of the initializer data which creates an unresolvable circular dependency. Allowing address(0) to act as an alias for "self" allows configuring calls to easily be specified at initialization which is useful for people who want to deploy safes that enable certain modules/guards from the start.

Implementation Explanation

The expression uses a binary-OR combined with a multiplication to implement a branch-less version of the following statement:

to = to == address(0) ? address(this) : to;

Here's the reasoning for why or(to, mul(iszero(to), address()) is equivalent to the above ternary expression:

if (to == 0) { or(to, mul(address(), iszero(to))) } else /* to == [1, 2^160-1] */ {  or(to, mul(address(), iszero(to))) }
if (to == 0) { or(0 , mul(address(), 1)         ) } else /* to == [1, 2^160-1] */ {  or(to, mul(address(),     0     )) }
if (to == 0) { or(0 ,     address()             ) } else /* to == [1, 2^160-1] */ {  or(to,                    0      ) }
if (to == 0) {            address()               } else /* to == [1, 2^160-1] */ {     to                              }
github-actions[bot] commented 10 months ago

CLA Assistant Lite bot All contributors have signed the CLA ✍️ ✅

Philogy commented 10 months ago

I have read the CLA Document and I hereby sign the CLA

nlordell commented 10 months ago

Hey, thanks for the contribution. Personally, I think this is a very cool idea:

  1. Sending transactions to address 0 is not really useful in and of itself (you can always use another address to burn ETH such as 0xee..ee), so it doesn't really impose any practical restrictions on MultiSend contract usage
  2. Generally speaking it allows the Safe setup to do a lot of things (where you don't know the Safe address upfront, so you can use address(0) in the to to call enableModule multiple times for example instead of requiring a specialised contract to do so).
  3. The solution uses an elegant branchless programming trick and has a low gas overhead

The biggest drawback to the change is the requirement to deploy new MultiSend* contracts and update where they get used, but I do think that it is useful enough to merit the downsides.

mmv08 commented 10 months ago

Hey, thanks for the contribution. Personally, I think this is a very cool idea:

  1. Sending transactions to address 0 is not really useful in and of itself (you can always use another address to burn ETH such as 0xee..ee), so it doesn't really impose any practical restrictions on MultiSend contract usage
  2. Generally speaking it allows the Safe setup to do a lot of things (where you don't know the Safe address upfront, so you can use address(0) in the to to call enableModule multiple times for example instead of requiring a specialised contract to do so).
  3. The solution uses an elegant branchless programming trick and has a low gas overhead

The biggest drawback to the change is the requirement to deploy new MultiSend* contracts and update where they get used, but I do think that it is useful enough to merit the downsides.

We can merge this and include it in the upcoming 1.5 release.

@Philogy do you think you'd have time to add a couple of tests for your change?

coveralls commented 10 months ago

Pull Request Test Coverage Report for Build 6800828476


Totals Coverage Status
Change from base Build 6720462527: 0.0%
Covered Lines: 400
Relevant Lines: 409

💛 - Coveralls