whitelist replay deployer #4009
Conversation
api/coreservice.go
Outdated
| @@ -132,6 +132,8 @@ type ( | |||
| EVMNetworkID() uint32 | |||
| // ChainID returns the chain id of evm | |||
| ChainID() uint32 | |||
| // IsDeployerWhitelisted returns if the replay deployer is whitelisted | |||
| IsDeployerWhitelisted(address.Address) bool | |||
There was a problem hiding this comment.
for check the received action
There was a problem hiding this comment.
unnecessary to do filtering inside API
There was a problem hiding this comment.
i think we should? otherwise it enters workingset and throws error
similar to ChainID check, we filter and reject inside API as well (also check and throw error in workingset)
api/coreservice.go
Outdated
| // reject action if a replay tx is not whitelisted | ||
| deployer := selp.SrcPubkey().Address() | ||
| if selp.Encoding() == uint32(iotextypes.Encoding_ETHEREUM_UNPROTECTED) && !core.IsDeployerWhitelisted(deployer) { | ||
| return "", status.Errorf(codes.InvalidArgument, "replay deployer %v not whitelisted", deployer.Hex()) | ||
| } |
There was a problem hiding this comment.
Let's leave it to next PR: not sure we need to add a function to the blockchain interface just for filtering purpose.
There was a problem hiding this comment.
it is in blockchain interface for 2 reasons:
- workingset needs it when processing actions
- here core service use it for whitelist deployer check
or what is your suggestion?
blockchain/blockchain.go
Outdated
| Tip: tip, | ||
| ChainID: bc.ChainID(), | ||
| EvmNetworkID: bc.EvmNetworkID(), | ||
| IsDeployerWhitelisted: bc.config.IsDeployerWhitelisted, |
There was a problem hiding this comment.
not sure whether it is appropriate to add it to context in this way. btw, if it is added to context, no need to add IsDeployerWhitelisted to Blockchain
There was a problem hiding this comment.
add to context is for workingset to use when process actions, add to Blockchain is for coreservice to use, as explained above
or what is your suggestion?
There was a problem hiding this comment.
quote what you said above:
it is in blockchain interface for 2 reasons:
workingset needs it when processing actions
here core service use it for whitelist deployer check
blockchain/config.go
Outdated
| @@ -37,6 +37,7 @@ type ( | |||
| ID uint32 `yaml:"id"` | |||
| EVMNetworkID uint32 `yaml:"evmNetworkID"` | |||
| Address string `yaml:"address"` | |||
| ReplayDeployerWhitelist []string `yaml:"replayDeployerWhitelist"` | |||
There was a problem hiding this comment.
what if the field is added to genesis?
There was a problem hiding this comment.
genesis.Blockchain is also an option, but looking at the fields I feel it is more appropriate to add here?
and by adding to genesis.Blockchain, we still need the new func in coreservice and context
Codecov ReportAttention:
Additional details and impacted files@@ Coverage Diff @@
## master #4009 +/- ##
==========================================
+ Coverage 75.38% 76.18% +0.80%
==========================================
Files 303 338 +35
Lines 25923 28795 +2872
==========================================
+ Hits 19541 21938 +2397
- Misses 5360 5738 +378
- Partials 1022 1119 +97 ☔ View full report in Codecov by Sentry. |
blockchain/config.go
Outdated
| // IsDeployerWhitelisted returns whether the address is whitelisted for replay transaction | ||
| func (cfg *Config) IsDeployerWhitelisted(deployer address.Address) bool { | ||
| for _, v := range cfg.ReplayDeployerWhitelist { | ||
| if v[:3] == "io1" { |
There was a problem hiding this comment.
It's better to use a function to parse string to address, supporting ethAddr and ioAddr. Some similar code lies in ioctl/util/util.go:157.
dustinxie
force-pushed
the
check
branch
2 times, most recently
from
807be0d to
d3baef3
Compare
| return nil, err | ||
| } | ||
| } | ||
| // for replay tx, check against deployer whitelist | ||
| g := genesis.MustExtractGenesisContext(ctx) |
There was a problem hiding this comment.
This check maybe more suitable to be placed in the validate function, executed before the runAction function.
There was a problem hiding this comment.
validate() does not have actions as input, so cannot check the sender address is whitelisted or not
|
Description
(#3965) has enabled pre-EIP155 unprotected tx. To ensure safety against replay attack, we limit the deployer address to an expected whitelist.
Fixes #(issue)
Type of change
Please delete options that are not relevant.
How Has This Been Tested?
Please describe the tests that you ran to verify your changes. Provide instructions so we can reproduce. Please also list any relevant details for your test configuration
Test Configuration:
Checklist: