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.

Sign up for GitHub

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

Jump to bottom

Detect delegated sender authorization in CustomMessageCallProcessor #9814

Merged
merged 20 commits into from Nov 14, 2023
Merged

Detect delegated sender authorization in CustomMessageCallProcessor #9814

merged 20 commits into from Nov 14, 2023

Conversation

tinker-michaelj
Copy link
Collaborator

@tinker-michaelj tinker-michaelj commented Nov 10, 2023 •
edited

Description:

  • Closes Fix delegateContractIdRequiredForTransferInDelegateCall test from ContractCreateSuite #8717
  • Enables delegateContractIdRequiredForTransferInDelegateCall() as a @HapiTest.
  • On develop, the CustomMessageCallProcessor incorrectly checks whether the executing frame is a delegatecall to decide when to activate only delegatable_contract_id keys in a receiverSigRequired key structure.
    • But this just limits a contract's ability to use DELEGATECALL to transfer value to an account it controls, which does not make sense.
    • Instead we want to limit when a contract can use delegated code to transfer value to an account that it controls.
  • This PR fixes the mistake by adding a FrameUtils.acquiredSenderAuthorizationViaDelegateCall() method that instead tests if the executing frame acquired its sender authorization via delegatecall (i.e. whether its parent receiver address was running delegated code when it initiated the call represented by the executing frame).

Misc - delegates highly unwanted debug logging from TokenServiceApiImpl.

@tinker-michaelj
Add gas fees to ContractCreate and ContractCall record transactionFee…
fd1d0e9 
… fields

Signed-off-by: Michael Tinker <michael.tinker@swirldslabs.com>
@tinker-michaelj
Merge remote-tracking branch 'origin/develop' into 09663-inc-gas-in-t…
0f50b2b 
…xn-fee
@tinker-michaelj
Revert HtsCall change
fe55ec7 
Signed-off-by: Michael Tinker <michael.tinker@swirldslabs.com>
@tinker-michaelj
Remove unwanted log
207b036 
Signed-off-by: Michael Tinker <michael.tinker@swirldslabs.com>
@tinker-michaelj
Fix typo
6837011 
Signed-off-by: Michael Tinker <michael.tinker@swirldslabs.com>
@tinker-michaelj
fix unit test
dd7619d 
Signed-off-by: Michael Tinker <michael.tinker@swirldslabs.com>
@tinker-michaelj
intermezzo
057efd7 
Signed-off-by: Michael Tinker <michael.tinker@swirldslabs.com>
@tinker-michaelj
Add unit tests, prep for PR
7ec4e2d 
Signed-off-by: Michael Tinker <michael.tinker@swirldslabs.com>
@tinker-michaelj
Merge remote-tracking branch 'origin/develop' into 09775-fix-sc-creat…
742efe6 
…e-children
@tinker-michaelj
Remove unwanted line
a14d618 
Signed-off-by: Michael Tinker <michael.tinker@swirldslabs.com>
@tinker-michaelj
Merge remote-tracking branch 'origin/develop' into 09775-fix-sc-creat…
5e17a3e 
…e-children
@tinker-michaelj
Use ExternalizedRecordCustomizer
17306b4 
Signed-off-by: Michael Tinker <michael.tinker@swirldslabs.com>
@tinker-michaelj
fix unit test
cc50fce 
Signed-off-by: Michael Tinker <michael.tinker@swirldslabs.com>
@tinker-michaelj
Fix typo
35b3889 
Signed-off-by: Michael Tinker <michael.tinker@swirldslabs.com>
@tinker-michaelj
fix unit test
3711915 
Signed-off-by: Michael Tinker <michael.tinker@swirldslabs.com>
@tinker-michaelj
Use ExternalizedRecordCustomizer instead of UnaryOperator
5370081 
Signed-off-by: Michael Tinker <michael.tinker@swirldslabs.com>
@tinker-michaelj
Address reviewer comments
27c8a83 
Signed-off-by: Michael Tinker <michael.tinker@swirldslabs.com>
@tinker-michaelj
Require @nonnull customizer
9f79573 
Signed-off-by: Michael Tinker <michael.tinker@swirldslabs.com>
@tinker-michaelj
Detect delegated sender authorization in CustomMessageCallProcessor a…
679d689 
…nd restrict to delegatable_contract_id keys

Signed-off-by: Michael Tinker <michael.tinker@swirldslabs.com>
@tinker-michaelj tinker-michaelj requested review from a team as code owners November 10, 2023 02:11
@github-actions GitHub Actions
Copy link

github-actions bot commented Nov 10, 2023 •
edited

Node: Unit Test Results

    2 278 files  ±0      2 278 suites  ±0   1h 29m 55s ⏱️ + 2m 48s
118 385 tests +4  118 351 ✔️ +4  34 💤 ±0  0 ❌ ±0 
126 646 runs  +4  126 612 ✔️ +4  34 💤 ±0  0 ❌ ±0 

Results for commit 42e8314. ± Comparison against base commit af2f38e.

♻️ This comment has been updated with latest results.

@github-actions GitHub Actions
Copy link

github-actions bot commented Nov 10, 2023 •
edited

Node: E2E Test Results

    1 files      1 suites   20m 32s ⏱️
312 tests 312 ✔️ 0 💤 0 ❌
334 runs  334 ✔️ 0 💤 0 ❌

Results for commit 42e8314.

♻️ This comment has been updated with latest results.

@github-actions GitHub Actions
Copy link

github-actions bot commented Nov 10, 2023 •
edited

Node: Integration Test Results

280 tests   280 ✔️  28m 50s ⏱️
    5 suites      0 💤
    5 files        0 ❌

Results for commit 42e8314.

♻️ This comment has been updated with latest results.

@github-actions GitHub Actions
Copy link

github-actions bot commented Nov 10, 2023 •
edited

Node: HAPI Test Results

1 246 tests   865 ✔️  1h 22m 43s ⏱️
   166 suites  381 💤
   166 files        0 ❌

Results for commit 42e8314.

♻️ This comment has been updated with latest results.

Neeharika-Sompalli
Neeharika-Sompalli previously approved these changes Nov 10, 2023
View reviewed changes
Copy link
Member

@Neeharika-Sompalli Neeharika-Sompalli left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM as per hedera-base changes ! Thanks @tinker-michaelj

Base automatically changed from 09775-fix-sc-create-children to develop November 10, 2023 16:49
Neeharika-Sompalli
Neeharika-Sompalli approved these changes Nov 14, 2023
View reviewed changes
Copy link
Member

@Neeharika-Sompalli Neeharika-Sompalli left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM !

lukelee-sl
lukelee-sl approved these changes Nov 14, 2023
View reviewed changes
Copy link
Member

@lukelee-sl lukelee-sl left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@tinker-michaelj tinker-michaelj merged commit c194954 into develop Nov 14, 2023
14 of 15 checks passed
@tinker-michaelj tinker-michaelj deleted the 08717-fix-recSigReq-delegate-detection branch November 14, 2023 18:08
nickpoorman pushed a commit that referenced this pull request Nov 22, 2023
@tinker-michaelj @nickpoorman
Detect delegated sender authorization in CustomMessageCallProcessor (…
878948e 
…#9814)

Signed-off-by: Michael Tinker <michael.tinker@swirldslabs.com>
Signed-off-by: Nick Poorman <nick@swirldslabs.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@Neeharika-Sompalli Neeharika-Sompalli Neeharika-Sompalli approved these changes

@lukelee-sl lukelee-sl lukelee-sl approved these changes

@kimbor kimbor Awaiting requested review from kimbor kimbor is a code owner automatically assigned from hashgraph/hedera-services

@acuarica acuarica Awaiting requested review from acuarica acuarica is a code owner automatically assigned from hashgraph/hedera-smart-contracts

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Fix delegateContractIdRequiredForTransferInDelegateCall test from ContractCreateSuite
3 participants
@tinker-michaelj @Neeharika-Sompalli @lukelee-sl