Almost self-destructing SELFDESTRUCT → DEACTIVATE

[yoavw]

@axic

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

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?

@axic

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.

I’m with you on that.

@axic

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.

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.