Solidity: How can we mitigate the issues that can be caused by selfdestruct

[frangio]

We should discuss how Solidity could have prevented the newer Parity multisig fiasco.

It has been on my mind for a while that contracts meant to receive payments should never be selfdestructed. Actually the whole selfdestruct feature is semantically odd and a big security hole. It means "from now on all transactions sent to this smart contract will pass through without actually being processed by the smart contract".

Options to consider:
1) Emit a warning when selfdestruct is used in a contract which has payable functions.
2) Emit a warning when selfdestruct is used in a library.
3) Emit a warning whenever selfdestruct is used.
4) Remove selfdestruct except from inline assembly.

[federicobond]

Thanks for taking the time to think about this @frangio, because it's important. I have a slightly different take and would love to discuss it with everyone.

While the ability of the library to selfdestruct itself was the direct cause of the Parity bug, I think the mistaken assumption here was that nobody would call the library as a contract. It breaks our assumption of libraries as meant only to be used within a delegatecall. One solution for this would be to add a check that address != caller at the top of each library bytecode.

[frangio]

100% agree with adding that check, and with the observation of the mistaken assumption that caused the bug.

In the particular case of the Parity bug it would not have helped, though. The "library" was actually a Solidity contract, and the main contract was manually using delegatecall.

[VoR0220]

simple solution imo is to disallow selfdestruct in all libraries. It kind of defeats the purpose of libraries imo if they are destructible as they are meant to be permanent figures that are reusable.

[federicobond]

But having the library selfdestruct the contract that delegatecalls it is perfectly valid.

[frangio]

Yes, @federicobond's suggestion to require that the function isn't called in the library's context would remove the problem of selfdestructing the library itself.

[VoR0220]

@federicobond true. But could a contract derive whether or not it's a delegatecall or a regular call? I think it could (and if not there should be an EIP specifying some kind of safety mechanism here) but I'm not sure.

[federicobond]

Hmm, after doing some research it looks like it is not possible to get the address of the contract that is currently executing when inside a delegatecall. Can anyone confirm?

[federicobond]

Parity used a contract instead of a library because there is no way to declare non-constant fields inside libraries. While their solution makes sense for economic and storage efficiency reasons, it remains overly clever given our current understanding of EVM semantics.

[frangio]

@federicobond I was able to get the contract address of the execution context via the this variable in a library function.

[VoR0220]

it makes sense from that standpoint but at the same time it's just better to pay the cost and be secure than trying to get clever...evidently.

[federicobond]

I just don't want to overfit the solution to this particular problem. It's clear that many people find proxies and delegatecall very useful and it's a pattern that will remain very popular. How can we work at the language level to ensure they are done in the safest way possible?

[VoR0220]

I still say that unless the code is being injected into the contract, that libraries should forbid selfdestructs within them. It wouldn't have prevented the Parity problem, but it is something that should be paid attention to nonetheless imo.

Another option would be to ban the use of selfdestruct throughout the compiler unless you drop into assembly mode. This seems to be a fairly popular opinion in the wake of this because immutability. Personally not a fan of this approach but I can see why its taken to people.

[RorschachRev]

The core issue was multisig, which gave ownership to anyone who asked it. To prevent same fiasco a third time, ownership of public functions should be explicit in the compiler step. (Parity lost a lot of money a few months back, same line of code in their contract, same bug.) If you want to get even more explicit about permissions, the ownership of functions (public and private) should belong to groups, and a function can only call sub functions with the same degree of ownership. We could support this in EVM, but supporting it in the contract compilation would halt compilation without blocking anything. The actual compiler step could merge public and private functions that are identical, costing no extra gas despite extra words.

I am personally undecided about suicide in general, because Ethereum can't support current rate of growth for 3 years with or without suicide. Meanwhile a lot of money is lost.

[3esmit]

First of all, the killed contract was not a true library, instead a regular contract. This problem would affect contracts that are true libraries (the ones used in "using" keyword")?

[RorschachRev]

I don't see many ways of limiting programmer error. https://github.com/paritytech/parity/pull/6103/files

[3esmit]

I think the solution is awarness and warnings when using selfdestruct.

It's simple to fix the "library" contract to become safe from the first call, in Parity case you just would need to add this constructor to WalletLibrary:

function WalletLibrary() {
 initMultiowned([0x1deadbeef, 0x2deadbeef], 2);
}

or something like this, but calling the initMultiowned at library constructor.

[RorschachRev]

The attack was 2 issued commands: initMultiowned and kill. These were the only two messages sent to the contract by the attacker. He became owner, and then suicide the contract. If we come up with some method to prevent suicide, we limit the owner takeovers to theft only instead.

[RorschachRev]

I'll put this another way. After a well known exploit was devised against Gav's very commonly copied and pasted multisig contract, Parity decides to use the same code. They paste insecure code into a contract controlling over $100 million and get exploited. They fix the exploit. Someone else pastes the original insecure code into a shared smart contract library and labels it a UI change. Whoever did the code review didn't look to see "oh hey, this updates contracts!" and labeled the UI change approved. (I'm not sure I believe Parity's story in this aspect - I think they decided 5000 lines of code was too much to review and just published to manipulate prices. That may make them guilty of more crimes than criminal negligence.) Then a few months go by, someone notices the master Parity github has the insecure code again and takes action. Rather than steal like the first hacker, he decides to self destruct the entire company and their partners with an ownership takeover and a suicide.

The way to prevent this is not be incompetent. Failing that, the compiler could require the programmer to be more explicit about function ownership.

[axic]

Options to consider:

1. Emit a warning when selfdestruct is used in a contract which has payable functions.
2. Emit a warning when selfdestruct is used in a library.
3. Emit a warning whenever selfdestruct is used.

All these are good candidates for the Javascript static analyzer (currently part of Remix, but planned to be made into an independent library) cc @soad003.

I do think selfdestruct is a legitimate feature (as long as it is part of EVM). The main issue at hand is still using delegatecall without validating a contract against all its implications.

The real solution I think is to create a proper framework for factory contracts with shared code, this is being discussed in #2296 and #3185.

[3esmit]

@RorschachRev here is not the place for bashing, but pointing out solutions, most of your "technical' analysis is not helpful and I assume everyone here knows the obvious, please stop spamming.

By the way, what happened in this contract is what we call "unknown unknowns", they audited but didn't noticed this detail, and now someone spot it everyone thinks that "is obviously ridiculous" "how noone seen that", "where are the specialists?", this is why this was an unknown unknown, which now is known we are discussing how to prevent this type of human error how to never happen again.
Blockchain technology is new for most of developers, and the techinique explored by Parity is roughly new, just like everything in Ethereum, and just now in Byzantium it can be done correctly (before that we could not return dynamic data, this would affect the hability to return delegated web3 calls that are dynamic sized.
I myself was not aware of this bug in particular, and would have done in future some error like this, but I've always took care to calling the initialize function of my "library" contracts, just in case of unknown unknowns (regarding something unknown) see: https://etherscan.io/address/0xc0FFeEE61948d8993864a73a099c0E38D887d3F4
But anyway this contract don't have a selfdestruct (btw its not called suicide in solidity), I did as I told, just because "why leave it open to other init?"

I agree with @axic but also we should rise an warning for every use of selfdestruct, specially if contract does not have a constructor.

[axic]

The problem with raising warnings in the compiler is that they cannot be switched off. We only raise warnings for things which are deprecated and can be silenced by using the recommended way of the given feature.

For others which are meant to be silenceable, there is the static analyzer code.

[RorschachRev]

Sorry I wasn't trying to spam. Can we add vulnerability checks (done by some tools) into the static analyzer?

[soad003]

@axic i have some train time on Wednesday. I can try to build a warning into remix whenever selfdestruct is used, referring to this incident.

Imo it would be useful to provide the devs with a better overview about which functions a contract exports and under what conditions (preconditions) those functions can run (to make it easier to spot mistakes). Unfortunately, the definition of preconditions of functions is not done consistently in Solidity (require, modifiers, if statements with throw) which makes the automatic extraction (as well as reading contracts) a bit complicated (what about ada stlye pre and post conditions in solidity?). We could add the artificial not selfdestruct precondition to every function as soon as a selfdestruct call is found in the contract.

Remix:
Color(green: internal, red: public) coding of function names could help to set function visibility properly.

[VoR0220]

@soad003 Seems slightly off topic but I think that the coloring of function names is an excellent idea.

[chriseth]

@axic I'm not really sure whether selfdestruct is such a valid feature. I remember having warned about it already at devcon1: https://chriseth.github.io/notes/talks/safe_solidity/#/4

The problem is that selfdestruct combined with a contract that can receive ether always has a race condition.

[3esmit]

@chriseth actually selfdestruct can be useful for Library Contracts.

I'm exploring a concept of crafting library contracts contract that are intentionally owned and destructable by a watchdog contract, and the stub contract would check if the system contract contains code, if not it would delegate to recover address.

I know there is ways of placing the emergency stop in a logic in contract itself, however using the kill of contracts that are automatically replaced by a safe recover contract seems to be a nice idea.

Please, don't get away with selfdestruct like you did with volatile constants https://github.com/ethereum/solidity/issues/976 ! :laughing: Let's us explore it (not only for errors), although I'm fine with removing the keyword from solidity and having it only in assembly :wink:

[3esmit]

I've posted a raw (untested) prototype I'm working https://github.com/3esmit/Hive-Democracy/commit/2d5641be6b53159124710cf372fe873f841f0799 using selfdestruct in a contract meant to be a Library Contract.

The advantage of this method is that once the emergencyStop is run, it's impossible to do anything else in the contract, unless the Recoverer contract conditions are met (a call from consensusContract()) setting a new system :)

[axic]

Added a new related proposal to #3221.

[chriseth]

Is there still something we can do or can this be closed?

[RorschachRev]

Self destruct is the idea of making the blockchain have owner operated
garbage collection. I'd rather solve scalability and remove self destruct.

On Wed, Mar 7, 2018 at 2:27 PM, chriseth [e-mail] wrote:

Is there still something we can do or can this be closed?

—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
https://github.com/ethereum/solidity/issues/3178#issuecomment-371290815,
or mute the thread
https://github.com/notifications/unsubscribe-auth/AA0ZHd3p792avXiY0zxN4HD9Os3xzEYiks5tcFDMgaJpZM4QU939
.

[chriseth]

@RorschachRev I agree, and selfdestruct is even not properly incentivised, but this is an EVM issue and not too related to the high-level aspects of the language.

[myFairCatsSick]

I haven't been able to find who is allowed to call selfdestruct, only the contract owner? Otherwise this seems a bit scary. I would think the use of a one-time pad (placing some hash or encrypted blob that must be verified) to protect the call of self-destruct might help. Its not fancy but it seems like decent insurance in these cases of mass monetary loss.

[RorschachRev]

Insecure contract gave away ownership, new owner called self destruct.
Don't change the protocol when it wouldn't improve anything.

On Mon, Apr 9, 2018 at 12:34 PM, myFairCatsSick [e-mail]
wrote:

I haven't been able to find who is allowed to call selfdestruct, only the
contract owner? Otherwise this seems a bit scary. I would think the use of
a one-time pad (placing some hash or encrypted blob that must be verified)
to protect the call of self-destruct might help. Its not fancy but it seems
like decent insurance in these cases of mass monetary loss.

—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
https://github.com/ethereum/solidity/issues/3178#issuecomment-379850631,
or mute the thread
https://github.com/notifications/unsubscribe-auth/AA0ZHW7iFDLg1Ho6fXa_KTdLJZsoYROpks5tm6nHgaJpZM4QU939
.