Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix: Cancun SELFDESTRUCT semantics incorrect when beneficiary is contract itself and it isn't deleted #17478

Draft
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

david-bakin-sl
Copy link
Member

Description:

(See hashgraph/hedera-smart-contracts#936 for original issue.)

Issue reported for SELFDESTRUCT is that EIP-6780 clearly states that the target address for the sweep aka the beneficiary aka the obtainer (in our code for contract delete) can be the contract address itself and there are two cases:

  • SELFDESTRUCT different transaction than contract created - this is the case where (post-cancun) the contract is not self destructed - contract keeps its balance
    • our behavior: return error SELF_DESTRUCT_TO_SELF - this is a bug as the contract/account is not deleted so it should work; in fact SELFDESTRUCT should work even if the account holds native (non-HBAR) tokens

This PR fixes issue above, and reorganizes CustomSelfDestructOperation.execute to make all validation tests clear and before all state changes.

Related issue(s):

Fixes #17477

Notes for reviewer:

Checklist

  • Documented (Code comments, README, etc.)
  • Tested (unit, integration, etc.)

…ract itself and it isn't deleted

(See hashgraph/hedera-smart-contracts#936 for original issue.)

Issue reported for SELFDESTRUCT is that [EIP-6780](https://eips.ethereum.org/EIPS/eip-6780) clearly states that the target address for the sweep aka the beneficiary aka the obtainer (in our code for contract delete) can be the contract address itself and there are two cases:
- SELFDESTRUCT different transaction than contract created - this is the case where (post-cancun) the contract is _not_ self destructed - contract keeps its balance
  - our behavior: return error SELF_DESTRUCT_TO_SELF - this is a bug as the contract/account is _not_ deleted so it should work; in fact SELFDESTRUCT should work even if the account holds native (non-HBAR) tokens

Signed-off-by: David S Bakin <[email protected]>
@david-bakin-sl david-bakin-sl added the Hedera Smart Contract Service Issues related to the Hedera Smart Contract Service. label Jan 22, 2025
@david-bakin-sl david-bakin-sl added this to the v0.59 milestone Jan 22, 2025
@david-bakin-sl david-bakin-sl self-assigned this Jan 22, 2025
Copy link

codecov bot commented Jan 22, 2025

Codecov Report

Attention: Patch coverage is 28.30189% with 38 lines in your changes missing coverage. Please review.

Project coverage is 68.95%. Comparing base (bbdc054) to head (6f16858).

Files with missing lines Patch % Lines
...l/exec/operations/CustomSelfDestructOperation.java 34.09% 24 Missing and 5 partials ⚠️
.../contract/impl/state/DispatchingEvmFrameState.java 0.00% 7 Missing ⚠️
...service/contract/impl/state/ProxyWorldUpdater.java 0.00% 2 Missing ⚠️
Additional details and impacted files

Impacted file tree graph

@@             Coverage Diff              @@
##               main   #17478      +/-   ##
============================================
- Coverage     68.99%   68.95%   -0.04%     
+ Complexity    22770    22766       -4     
============================================
  Files          2619     2619              
  Lines         98268    98298      +30     
  Branches      10184    10192       +8     
============================================
- Hits          67796    67785      -11     
- Misses        26644    26680      +36     
- Partials       3828     3833       +5     
Files with missing lines Coverage Δ
...service/contract/impl/hevm/HederaWorldUpdater.java 100.00% <ø> (ø)
...service/contract/impl/state/ProxyWorldUpdater.java 93.28% <0.00%> (-1.42%) ⬇️
.../contract/impl/state/DispatchingEvmFrameState.java 86.29% <0.00%> (-3.18%) ⬇️
...l/exec/operations/CustomSelfDestructOperation.java 49.25% <34.09%> (-46.40%) ⬇️

Impacted file tree graph

Copy link

Coverage summary from Codacy

See diff coverage on Codacy

Coverage variation Diff coverage
-0.03% (target: -1.00%) 37.74%
Coverage variation details
Coverable lines Covered lines Coverage
Common ancestor commit (bbdc054) 98051 71473 72.89%
Head commit (6f16858) 98081 (+30) 71467 (-6) 72.87% (-0.03%)

Coverage variation is the difference between the coverage for the head and common ancestor commits of the pull request branch: <coverage of head commit> - <coverage of common ancestor commit>

Diff coverage details
Coverable lines Covered lines Diff coverage
Pull request (#17478) 53 20 37.74%

Diff coverage is the percentage of lines that are covered by tests out of the coverable lines that the pull request added or modified: <covered lines added or modified>/<coverable lines added or modified> * 100%

See your quality gate settings    Change summary preferences

Codacy stopped sending the deprecated coverage status on June 5th, 2024. Learn more

@david-bakin-sl david-bakin-sl changed the title fix: Cancun SELFDESTRUCT semantics incorrect when beneficiary is coract itself and it isn't deleted fix: Cancun SELFDESTRUCT semantics incorrect when beneficiary is contract itself and it isn't deleted Jan 22, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Hedera Smart Contract Service Issues related to the Hedera Smart Contract Service.
Projects
None yet
1 participant