\
This property was surrendered via DELEGATECALL proxies. I presume you wish to keep those, but the behavior of a contract can change significantly without changing its code.
Nobody can trust EVM code without reading it. The same off-chain processes that check for DELEGATECALL upgradeability would need to check for SELFDESTRUCT. Neutering SELFDESTRUCT doesn’t change this.
I believe the word “trust” here is being used a bit differently than normal. In this case, it means that things like tooling, consensus, etc. can rely on the set of bytes written to the code at an address not changing.
@wjmelements 's comment about DELEGATECALL seems to me still hold true in these examples. could you @MicahZoltu could you ellaborate more examples about in what scenarios such tooling, consensus will survive the ability to change behavior without change code enabled by DELEGATECALL.To me behavior immutability is broken by DELEGATECALL regardless of whether SELFDESTRUCT exist.
Yes, behavior immutability is entirely within the control of the contract author. Even without delegate call this would still largely be true (just harder). When we say “the code cannot change” we mean that it literally cannot change, not that its behavior is not dynamic. When your building your tree structure of state, for example, you may be able to do some optimizations if you know that a particular bit of data cannot/will not change.
I have been out of the verkle tree loop for a while, but I believe whether this assertion can be made has a notable impact on that as well.
This is a very subtle change. How do we convince ourselves that this is safe for existing contracts?
You’re right - we can’t. In fact it’s likely that it isn’t 
For example, consider a multisig like Safe, that has signers and modules, with the following flow:
Before this EIP, the result is that only signer1 is a valid signer. After this EIP, {signer2,signer3} are also valid signers.
Besides, it opens up interesting new ways to rugpull/backdoor. E.g. deploy a token, mint yourself a large balance, selfdestruct, redeploy. Now totalSupply is reset, everything looks good, but in fact you still have the large balance (possibly higher than the totalSupply). Some backdoors would be almost impossible to detect, if delegating to a library that gets selfdestructed and replaced.
This example is not correct, because the safe initialisation not only sets an array of signers, but also the count. See this code.
This is a more realistic example, unlike the safe above. I personally would not trust any contract which selfdestructs, nor do I trust proxy contracts much. That being said, it is already possible to hide intent in various ways.
Besides, there are some other potential options in making the old storage slots shadowed (inaccessible entirely) with hashing the keys with a revival-nonce. Not that I am fond of that approach.
Hmmm, I believe it is correct because the count is not checked when the signer is used. Neither in isOwner, nor in checkNSignatures.
During signatures check, I think the default flow for a normal signature would reach currentOwner = ecrecover(dataHash, v, r, s) in line 299, so currentOwner will be set to the left-over signer (despite the count). And then it will pass the require(currentOwner > lastOwner && owners[currentOwner] != address(0) && currentOwner != SENTINEL_OWNERS, "GS026"); in line 301 because owners[currentOwner] has been set before the DEACTIVATE happened. Therefore the signature will be counted towards requiredSignatures.
If requiredSignatures is set to 3, and there were 3 signers before DEACTIVATE, who are no longer valid signers in the current safe, they’ll be able to pass the signature check without any of the valid signers participating.
The count is only used during getOwners() which is never called on-chain. This makes the problem worse, because the “shadow owners” remain hidden when getOwners() is checked in the UI.
Am I missing some check that would prevent this?
I’m with you on that.
Yes, I was also considering the option of hashing it in the compiler, so that slots are not reused. But this requires an opcode for accessing the nonce, and also not resetting the nonce to 0 on revival, but to a random number. I actually think this would be a good addition to the compiler (if we add the EXTNONCE opcode - maybe that’s another good use case for EIP-4672). It would also prevent similar issues with proxies. What would be the downside of that approach?
To clarify, I don’t mean using EXTNONCE every time a mapping is accessed. The nonce may change when creating additional contracts. I mean saving it as immutable during construction, so when a contract is “revived” it’ll have a different storage base. And maybe instead of hashing it, we would add it, so that it also affects simple variables and arrays, not just mappings.
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.
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.
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
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.
@axic mind reviewing the following?
It modifies this EIP to make deactivate effectively clear a contract’s storage.
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).
This has not been addressed yet, right?
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.
No worries. Note that I created a competing EIP for it:
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.