-
Notifications
You must be signed in to change notification settings - Fork 203
Restrict EVM access to EOAs with proven malicious activity #8272
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
Conversation
|
Important Review skippedAuto reviews are disabled on base/target branches other than the default branch. Please check the settings in the CodeRabbit UI or the You can disable this status message by setting the Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Codecov Report❌ Patch coverage is
📢 Thoughts on this report? Let us know! |
ee53efc to
baae296
Compare
fvm/evm/stdlib/contract.cdc
Outdated
| gasLimit: gasLimit, | ||
| value: value.attoflow | ||
| ) as! Result | ||
| panic("EVM transactions are temporarily disabled") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this will prevent script executions from calling into EVM right?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nevermind. that uses dryCall
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah, we have dryCall for that. I have only updated APIs that mutate the EVM state.
baae296 to
4a3ef39
Compare
|
@m-Peter do we also need to disable?
|
|
@peterargue All the bridge related operations, under the hood use
|
4a3ef39 to
11d2be4
Compare
This comment was marked as outdated.
This comment was marked as outdated.
dec905a to
4ecb6fa
Compare
| /// This is only a temporary measure and will be removed immediately | ||
| /// after the remediation of the illicit tokens | ||
| // in the Dec 2025 security incident is complete. | ||
| /// This function can only be called from the `FlowServiceAccount` contract, | ||
| /// and only from the holder of `FlowServiceAccount.Administrator` resource. | ||
| access(account) | ||
| fun reclaimFundsFromAttackerEOAs(from: String, to: String, amount: UInt): Result { | ||
| return InternalEVM.call( | ||
| from: EVM.addressFromString(from).bytes, | ||
| to: EVM.addressFromString(to).bytes, | ||
| data: [], | ||
| gasLimit: 1_000_000, | ||
| value: amount | ||
| ) as! Result | ||
| } | ||
|
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This should be removed, right?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think it could be - I believe all EOAs have been reclaimed, but we can also do this bit later, not critical IMO
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We can remove this, if we are not interested in claiming the 1,005 USDF from the EOA address. However, this can be done later on I think. Because we also have to update the FlowServiceAccount, it is currently referencing this function.
Suggestion to 8272: move the msg.From check to proc.run
Co-authored-by: Bastian Müller <[email protected]>
| // Ethereum addresses are case-insensitive in hex representation | ||
| // but gethCommon.HexToAddress normalizes to lowercase | ||
| addr1 := gethCommon.HexToAddress("0x2e7C4b71397f10c93dC0C2ba6f8f179A47F994e1") | ||
| addr2 := gethCommon.HexToAddress("0x2e7c4b71397f10c93dc0c2ba6f8f179a47f994e1") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Added some test case to double check the same address in different case format will still be rejected.
fvm/evm/emulator/emulator.go
Outdated
| } | ||
|
|
||
| // Restrict access to EVM, for EOAs with proven malicious activity | ||
| if slices.Contains(restrictedEOAs, msg.From) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
should this check still be here now that it's done in run?
fvm/evm/emulator/emulator.go
Outdated
| txType uint8, | ||
| ) (*types.Result, error) { | ||
| // Restrict access to EVM, for EOAs with proven malicious activity | ||
| if isRestrictedEOA(msg.From) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this is also called within dryRun
https://github.com/onflow/flow-go/blob/mpeter/disable-evm-state-mutation/fvm/evm/emulator/emulator.go#L351
I'm guessing that is fine, but double checking
…h-AN-compatibility-for-v0.44.13
…h-AN-compatibility-for-v0.44.14 to Restrict access to EOAs only
…y-for-v0.44.14 add AN compatibility for v0.44.14 - restrict only EOAs
This will also resume all EVM operations for the public.