Solidity: solc >= 0.5.0 incorrectly requires selfdestruct parameter is payable

[wjmelements]

The EVM does not require that the recipient of selfdestruct ether can accept it. There is no way to reject it, so nonpayable addresses are just as valid as payable addresses.

[chriseth]

While this is true, Solidity employs many techniques to flatten out some of the weird edge cases of the EVM. I think we should strive to enforce payable as much as possible, even if ether can in principle be sent to non-payable addresses.

Do you think the requirement of payable makes people think that the fallback function is executed for a selfdestruct?

[wjmelements]

It may lead a developer to believe that selfdestruct would fail for a nonpayable contract. If payable has any meaning at all, it should not apply in this context.

[chriseth]

payable/nonpayable is also not enforced at other locations. For a plain address, for example.

[wjmelements]

payable/nonpayable is enforced tyrannically in all locations. It is so bad that if you want to test this case you have to convert address to uint160 and back.

[wjmelements]

Here is a case of improper enforcement

[wjmelements]

You cannot transfer ether to an address if it is not payable. You cannot cast a nonpayable contract to a payable contract, or vice-versa.

[wjmelements]

Here is the workaround I am using. It is annoying that the spec of the language does not line up with the EVM, and that the workaround is to abandon typing.

[chriseth]

Do you have a link to the full context of the error message and source snippet you provide here? Another solution would be to use

function destroyAndSend(address payable _recipient) public {
  selfdestruct(_recipient);
}

I assume you did not go with that because then you would have to introduce conversions to payable higher up in the call stack. Still, it would be great to take a look!

On another note: Starting with Solidity 0.6.0, you can write selfdestruct(payable(_recipient)).

[wjmelements]

Do you have a link to the full context of the error message and source snippet you provide here?

https://github.com/trusttoken/true-currencies/blob/ff29589ef787d2c624656cd3bd5846df4474b711/test/TokenController.test.js#L591

Another solution would be to use

function destroyAndSend(address payable _recipient) public {
  selfdestruct(_recipient);
}

I assume you did not go with that because then you would have to introduce conversions to payable higher up in the call stack. Still, it would be great to take a look!

Right. The test case is necessary to verify that code that can extract ether from a nonpayable contract works. Line coverage is very important for us in testing.

On another note: Starting with Solidity 0.6.0, you can write selfdestruct(payable(_recipient)).

That's a delightful improvement, though not as good as selfdestruct(recipient).

[leonardoalt]

@wjmelements did I get it right that your suggestion is to relax the nonpayable check for selfdestruct?

[wjmelements]

Right

On Fri, Nov 29, 2019 at 2:13 AM Leonardo [e-mail] wrote:

@wjmelements https://github.com/wjmelements did I get it right that
your suggestion is to relax the nonpayable check for selfdestruct?

—
You are receiving this because you were mentioned.

Reply to this email directly, view it on GitHub
https://github.com/ethereum/solidity/issues/7727?email_source=notifications&email_token=AAGDGVKJQGHYHI2P2FM3YITQWDTK7A5CNFSM4JOBXES2YY3PNVWWK3TUL52HS4DFVREXG43VMVBW63LNMVXHJKTDN5WW2ZLOORPWSZGOEFOOTSI#issuecomment-559737289,
or unsubscribe
https://github.com/notifications/unsubscribe-auth/AAGDGVM252AQ7IZ2DTNTIULQWDTK7ANCNFSM4JOBXESQ
.

[leonardoalt]

@chriseth I agree with it and would like to discuss it

[chriseth]

@axic do you have an opinion on this?

[chriseth]

While indeed, the EVM does not require "payable" and that having "payable" might suggest that the receive function is called on the target, the fact that not requiring "payable" would help in this situation. In general, "selfdestruct" is a rather complicated process and the "payable" part is not the most complicated one. In a team discussion, we concluded that none of the two solutions is clearly better and it might be more consistent to require "payable" whenever there is ether transferred, even if the receive function is not called.

[wjmelements]

@chriseth This is a clear inconsistency in the meaning of payable. Yet another reason to hate solidity.

[chriseth]

:(

[leonardoalt]

@chriseth This is a clear inconsistency in the meaning of payable. Yet another reason to hate solidity.

@wjmelements Just curiosity, what are the others?

[wjmelements]

@chriseth This is a clear inconsistency in the meaning of payable. Yet another reason to hate solidity.

@wjmelements Just curiosity, what are the others?

• The need to upcast contracts to address in >=0.5
• mload(0x40)
• The never-optimized universal storage layout, particularly that multi-layer mappings use multiple sha3

Of course there are things I like about it as well (uint256 constant-suffixes, inline asm, and fallback functions), and it's the best evm language at this time by far, but that means living with its flaws if you are a contract dev.

[leonardoalt]

I guess we all live with the flaws and things we dislike about the programming languages we use the most.

The third point though, it's a security feature. If you use a single hash of all the keys for a multi-d mapping access you can easily get collisions.

[wjmelements]

The third point though, it's a security feature. If you use a single hash of all the keys for a multi-d mapping access you can easily get collisions.

With a different scheme the field identifier would be enough to prevent collisions. For example,
sha3(uint256(field_id) . uint256(key1) . uint256(key2) . ... . uint256(keyN)). You wouldn't be able to tightly pack the keys but that's fine since MSTORE writes 32 bytes. A further benefit is that you could reuse this memory no matter what changed in the query. If you went from looking up an account balanceOf to looking up their allowance you wouldn't have to rewrite the account key. Since SHA3 is billed by word size you don't get much benefit from packing unless the keys are bytes4 etc but that's quickly trumped by the additional SHA3s.

[chriseth]

The main feature is actually "partial evaluation" - you can refer to m[x] as y and pass it on across function calls and then use y[t] to do the next lookup.

[chriseth]

... AND the type of this y is just the type if m[x] and not something like partially_evaluated_m.