Almost self-destructing SELFDESTRUCT → DEACTIVATE

[axic]

@jwasinger

The wording of the EIP implies that setting the nonce to 2^64-1 happens immediately upon calling DEACTIVATE. In my opinion, this should be moved to happen at the end of the transaction similarly to how SELFDESTRUCT works.

Good catch! I wonder however if it would make sense changing the behaviour to this, i.e. currently selfdestructed contracts can be called after they are selfdestructed as long as we didn’t exit the transaction frame.

[dror]

@axic

The safe uses a “sentinel” field to trail the owner array. See SENTINEL_OWNERS here

The sentinel is only a marker of the “end of the linked list”. An owner is active if its entry in the “owners” is non-zero and non-sentinel

The line you quote is inside the loop of checking owners.
What it checks is that the current address is unique (above previous one), and that it has SOME value in the owners hash (not zero and not “sentinel”).
The check does NOT check this entry is indeed in the linked-list of current owners as returned by the getOwners()
So you have a safe with (say) 3 owners, and now you “deactivate” and ressurect it (without clearing the storage), those 3 are now “shaddow” owners of the safe, and can participate in a multisig, even though they are not returned by “getOwners()”
The same is true for modules: if a safe has a module installed before such “reset”, this module is still eligible to make calls on safe, even though it is not returned by getModulesPaginated

The moral is that it is very unsafe to use “dirty” storage for any purpose, as contracts assume storage is clear when initiated.

[yoavw]

@axic

The safe uses a “sentinel” field to trail the owner array. See SENTINEL_OWNERS here and here .

As @dror wrote below, the SENTINEL is not a security check, just the terminator of the owners linked list. It is only used when traversing the list. It is used in getOwners() but not during isOwner() or checkNSignatures().

Transactions speak louder than words, so here’s a goerli Safe with some dirty storage. As you can see, you (axic.eth) are the only owner of this safe. It’s a perfectly normal GnosisSafeProxy created by GnosisSafeProxyFactory, and the implementation is set to the usual GnosisSafeL2 singleton.

Observe the safe’s history. You can see a successful transaction with the data “Hello world!” to axic.eth, which obviously you never signed. It was sent by one of the shadow-owners from the dirty storage.

If anyone wants to try it, this safe has 21 invisible signers, including all the default hardhat accounts (e.g. 0xf39fd6e51aad88f6f4ce6ab8827279cfffb92266). You can’t see any of these signers by observing the Safe, but they can send transactions anyway:

>>> bad_safe = Contract.from_abi('BadSafe', '0xD23e1E5F40Cae07E56E30Cdc710d9b107abD60DB', safe_singleton.abi)
>>> bad_safe.getOwners()
("0x068484F7BD2b7D7C5a698d89e75ddcaf3a92B879")
>>> bad_safe.isOwner('0xf39fd6e51aad88f6f4ce6ab8827279cfffb92266')    # The 1st hardhat account, a shadow owner.
True
>>> sig = encode_abi_packed(['bytes12', 'address', 'bytes32', 'uint8'], [web3.toBytes(hexstr='0x0'), '0xA2d74Ff1a49B8f89aC784a524a468b45f114de68', web3.toBytes(hexstr='0x0'), 1])
>>> bad_safe.execTransaction(bad_safe.getOwners()[0], 0, web3.toBytes(text='Hello world!'), 0, 0, 0, 0, '0x0000000000000000000000000000000000000000', '0x0000000000000000000000000000000000000000', sig, {'from':'0xA2d74Ff1a49B8f89aC784a524a468b45f114de68'}).info()
Transaction sent: 0xb09dcce42c3a6c419eda1a2f9715b87c6f5dd4b73c5a643fe13809509cf91dd5
  Gas price: 0.000189426 gwei   Gas limit: 81376   Nonce: 2
  BadSafe.execTransaction confirmed   Block: 8119125   Gas used: 73317 (90.10%)

Transaction was Mined 
---------------------
Tx Hash: 0xb09dcce42c3a6c419eda1a2f9715b87c6f5dd4b73c5a643fe13809509cf91dd5
From: 0xA2d74Ff1a49B8f89aC784a524a468b45f114de68
To: 0xD23e1E5F40Cae07E56E30Cdc710d9b107abD60DB
Value: 0
Function: BadSafe.execTransaction
Block: 8119125
Gas Used: 73317 / 81376 (90.1%)

Events In This Transaction
--------------------------
└── BadSafe (0xD23e1E5F40Cae07E56E30Cdc710d9b107abD60DB)
    ├── SafeMultiSigTransaction
    │   ├── to: 0x068484F7BD2b7D7C5a698d89e75ddcaf3a92B879
    │   ├── value: 0
    │   ├── data: 0x48656c6c6f20776f726c6421
    │   ├── operation: 0
    │   ├── safeTxGas: 0
    │   ├── baseGas: 0
    │   ├── gasPrice: 0
    │   ├── gasToken: 0x0000000000000000000000000000000000000000
    │   ├── refundReceiver: 0x0000000000000000000000000000000000000000
    │   ├── signatures: 0x000000000000000000000000a2d74ff1a49b8f89ac784a524a468b45f114de68000000000000000000000000000000000000000000000000000000000000000001
    │   └── additionalInfo: 0x0000000000000000000000000000000000000000000000000000000000000000000000000000000000000000a2d74ff1a49b8f89ac784a524a468b45f114de680000000000000000000000000000000000000000000000000000000000000001
    └── ExecutionSuccess
        ├── txHash: 0x5bd5eedbb6f5c66f781f1ffc22b6ed90c06fdbf124a71f1d07bdaa2e06d8c759
        └── payment: 0

@axic

This example is not correct, because the safe initialisation not only sets an array of signers, but also the count.

Hopefully I established that it is. But I think we can make DEACTIVATE (and proxies in general) safer by using the compiler to add an offset to all storage access. An extension of what I wrote above regarding EXTNONCE. I’ll write a proposal for it later.

[Pandapip1]

@axic mind reviewing the following?

Update EIP-6046: Propose alternative deactivation

It modifies this EIP to make deactivate effectively clear a contract’s storage.

[axic]

@Pandapip1 Sorry I totally missed these notifications.

[axic]

@axic

@dankrad

It’s an interesting idea, but this introduces another pitfall: Unlike the old behaviour, storage does not get cleared.

This is listed in the backwards compatibility section of the EIP. Various ideas were discussed to work around this, with causing even more storage use, for example: hash storage keys one more time with the special nonce (in this case a range would need to be allocated), to avoid collisions. This also would make accessing old ones after revival impossible.

Published a version of this yesterday on the R&D discord: DEACTIVATE (aka SELFDESTRUCT) with old-storage shadowing - HackMD

This fixes the problem of “pre-existing storage” by shadowing old storage. From a users’ perspective there is no change how selfdestruct behaves and so it won’t cause any risk of bricking contracts.

This version is mostly made with Verkle trees in mind, it’s goal is to remove this last bottleneck for Verkle trees, and in fact to minimize number of changes one should consider introducing it together with Verkle trees and not before (see the account changes).

[rmeissner]

@wjmelements

One other matter for this approach to consider is how EXTCODECOPY and EXTCODEHASH should work.

This has not been addressed yet, right?

[axic]

Not sure what is unclear about this? When the account is “deactivated” (i.e. high nonce) then it is non-existing, otherwise it exists. For both existing and non-existing accounts the rules of EXTCODE* is well defined.

[Pandapip1]

No worries. Note that I created a competing EIP for it:

EIP-6190: Verkle-compatible SELFDESTRUCT

If you’re interested in pursuing that path, I would be willing to withdraw EIP-6190 if the content of it is merged into EIP-6046.