Fix/onramp allowlist race condition #1480
Conversation
…ndition Signed-off-by: 0xsuryansh <[email protected]>
Signed-off-by: 0xsuryansh <[email protected]>
LCOV of commit
|
0xsuryansh
requested review from
makramkd,
elatoskinas,
RayXpub and
a team
as code owners
| @@ -91,6 +91,7 @@ contract OnRamp is IEVM2AnyOnRampClient, ITypeAndVersion, OwnerIsCreator { | |||
| struct DestChainConfigArgs { | |||
| uint64 destChainSelector; // Destination chain selector | |||
| IRouter router; // Source router address | |||
| bool allowListEnabled; // Boolean indicator to specify if allowList check is enabled | |||
There was a problem hiding this comment.
let's add struct packing comments, they should have been there already but the previous person also missed them :)
There was a problem hiding this comment.
# Conflicts: # contracts/gas-snapshots/ccip.gas-snapshot # core/gethwrappers/ccip/generated/onramp/onramp.go # core/gethwrappers/ccip/generation/generated-wrapper-dependency-versions-do-not-edit.txt
Signed-off-by: 0xsuryansh <[email protected]>
Signed-off-by: 0xsuryansh <[email protected]>
Signed-off-by: 0xsuryansh <[email protected]>
|
Added a test for setting a new chain with |
Signed-off-by: 0xsuryansh <[email protected]>
# Conflicts: # contracts/gas-snapshots/ccip.gas-snapshot # core/gethwrappers/ccip/generation/generated-wrapper-dependency-versions-do-not-edit.txt
Signed-off-by: 0xsuryansh <[email protected]>
| IRouter router; // Source router address | ||
| uint64 destChainSelector; // ─╮ Destination chain selector | ||
| IRouter router; // │ Source router address | ||
| bool allowListEnabled; //─────╯ Boolean indicator to specify if allowList check is enabled |
There was a problem hiding this comment.
q: since this is now part of dest chain config args - do we still need it in allowListConfigArgs? There are now 2 ways to change the allowlist - is this intended?
There was a problem hiding this comment.
This just enables it when a new chain it set, to prevent unauthorised access to a private chain. The other method is still required to set the actual permitted addresses
There was a problem hiding this comment.
For the first option it makes sense, for the second option - we can always batch 2 MCMS calls:
setDestChainConfigArgssetAllowlistConfigArgs
Seems slightly redundant to have the flag twice
|
Motivation
When the
onRamp::_applyDestChainConfigUpdatesis invoked in the constructor, theallowListEnabledfield of the destination chain configuration is not set directly. As a result, if an onRamp with an allow list is required, the deployment must be bundled with a call toonRamp::applyAllowListUpdatesSolution
Directly sets the
allowListEnabledfield during the constructor execution.