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

allow transfer if call is restricted #2553

Closed
wants to merge 3 commits into from

Conversation

ramacarlucho
Copy link
Collaborator

@ramacarlucho ramacarlucho commented May 9, 2024

Description

Currently in the mono antehandler during the message validation, we check that the CREATE or CALL OP codes are restricted, and the validation fails in case it is.
With the addition of the permissioned evm, the signer of the transaction can be whitelisted an allowed the operation. In the first implementation, the check for a whitelisted address was performed during execution. With this approach we add the check for whitelist address in the antehandler. This requires a small refactor to check the permissions after the msg signature (since we need to know who sends the message)

On base denom transfers
If we restrict the CALL opcode, even the aevmos coin transfer will be restricted. So only cosmos txns will be available, and this is not very useful.
To allow base denom transfers we add the check txdata is empty, to verify that is not a contract call. But this does not cover the case of deposit or fallback. So to prevent this, we simply block all transactions which recipient is a contract.

Closes: EVM-148


Author Checklist

All items are required. Please add a note to the item if the item is not applicable and
please add links to any relevant follow up issues.

I have...

  • tackled an existing issue or discussed with a team member
  • left instructions on how to review the changes
  • targeted the correct branch (see PR Targeting)

Reviewers Checklist

All items are required.
Please add a note if the item is not applicable
and please add your handle next to the items reviewed
if you only reviewed selected items.

I have...

  • added a relevant changelog entry to the Unreleased section in CHANGELOG.md
  • confirmed all author checklist items have been addressed
  • confirmed that this PR does not change production code
  • reviewed content
  • tested instructions (if applicable)
  • confirmed all CI checks have passed

@ramacarlucho ramacarlucho requested a review from a team as a code owner May 9, 2024 18:45
@ramacarlucho ramacarlucho requested review from facs95 and MalteHerrmann and removed request for a team May 9, 2024 18:45
@github-actions github-actions bot added the tests label May 9, 2024
@@ -39,6 +39,11 @@ func checkDisabledCreateCall(
permissions *evmtypes.AccessControl,
) error {
to := txData.GetTo()
data := txData.GetData()
// If its not a contract creation or contract call this check is irrelevant
Copy link
Contributor

Choose a reason for hiding this comment

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

But what about the fallback and receive functions of a contract?
An EOA can trigger a contract call by just sending funds to it.

We could identify this by checking if the to address is a contract, e.g. checking if the address has code

Copy link
Contributor

Choose a reason for hiding this comment

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

this an important point that should be taken into consideration

Copy link
Contributor

Choose a reason for hiding this comment

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

I suggest for contracts checking the method ID from the call data (i.e the first 4 bytes) and comparing it with the receive and fallback bytes

Copy link

linear bot commented May 14, 2024

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants