fix: [C01, M01] Remove EIP-1271 signature recovery support and add destination chain ID to relay data #79
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
…ain ID to relay data
chrismaree
reviewed
Mar 9, 2022
| uint256 fillAmount, | ||
| uint256 repaymentChainId, | ||
| uint256 originChainId, | ||
| uint256 destinationChainId, |
Member
There was a problem hiding this comment.
if you do this then please also add the originChainId to FundsDeposited
chrismaree
reviewed
Mar 9, 2022
| import { toBNWei, SignerWithAddress, Contract, ethers, seedWallet, toBN } from "../utils"; | ||
| import { constructRelayParams, warmSpokePool, sendRelay } from "./utils"; | ||
| import * as constants from "../constants"; | ||
| import { spokePoolFixture, enableRoutes } from "../fixtures/SpokePool.Fixture"; |
chrismaree
approved these changes
Mar 9, 2022
Member
chrismaree
left a comment
chrismaree
left a comment
There was a problem hiding this comment.
LGTM! makes sence as a change
mrice32
reviewed
Mar 9, 2022
Contributor
mrice32
left a comment
mrice32
left a comment
There was a problem hiding this comment.
Looks great! Two minor comments
contracts/SpokePool.sol
Outdated
| // This function is isolated and made virtual to allow different L2's to implement chain specific recovery of | ||
| // signers from signatures because some L2s might not support ecrecover, such as those with account abstraction | ||
| // like ZKSync. | ||
| // like ZKSync. To be safe, consider always reverting this function for L2s where ecrecover is different from how |
Contributor
There was a problem hiding this comment.
Does ZKSync not support standard ecrecover?
Member
Author
There was a problem hiding this comment.
I guess they do now, but its a special case so we need to be careful when integrating with zksync: https://v2-docs.zksync.io/dev/testnet/status.html#currently-supported-features
contracts/SpokePool.sol
Outdated
| address destinationToken, | ||
| uint256 amount, | ||
| uint256 originChainId, | ||
| uint256 destinationChainId, |
Contributor
There was a problem hiding this comment.
Maybe we should omit this to save calldata and assume that the current chainId is the one they intended? Thoughts?
mrice32
approved these changes
Mar 10, 2022
chrismaree
approved these changes
Mar 11, 2022
nicholaspai
added
the
OZ Audit - March
nicholaspai
changed the title
This was referenced Apr 6, 2022
nicholaspai
changed the title
This was referenced May 3, 2022
Merged
This was referenced May 17, 2022
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Issue:
multiple tokens and spoke pools. A valid root bundle would ensure the poolRebalanceRoot has a leaf
for every spoke chain. When this rebalance leaf is processed, the slowRelayRoot will also be sent to
the corresponding spoke pool.
batch across the whole system. When the slow relay is executed, the Spoke Pool does not filter on the
destination chain id, which means that any slow relay can be executed on any spoke chain where the
Spoke Pool has sufficient funds in the destinationToken. Consider including the destination chain ID in
the slow relay details so the Spoke Pool can filter out relays that are intended for other chains.
intention. The message is verified on the origin chain before emitting the event that notifies relayers,
and verified again on the destination chain before the new fee can be used to fill the relay. If the
depositor used a static ECDSA signature and both chains support the ecrecover opcode, both
verifications should be identical. However, verification uses the OpenZeppelin Signature Checker
library, which also supports EIP-1271 validation for smart contracts. If the smart contract validation
behaves differently on the two chains, valid contract signatures may be rejected on the destination
chain. A plausible example would be a multisignature wallet on the source chain that is not replicated
on the destination chain.
RequestedSpeedUpDeposit event in the off-chain UMIP specification, so that relayers that comply with
the event would be reimbursed. This mitigation would need a mechanism to handle relayers that
incorrectly fill relays with excessively large relayer fees, which would prevent the recipient from
receiving their full payment. Alternatively, consider removing support for EIP-1271 validation and
relying entirely on ECDSA signatures.
Resolution:
destinationChainIdto the Relay Data is important so that slow relays can be associated deterministically to relays on a specific chain. Imagine a scenario where a slow relay to fill a relay on chain 100 is added to theslowRelayRoot. This slow relay is valid so the root bundle including theslowRelayRooton the HubPool successfully passes liveness. Now, anyone can submit that slow relay leaf to a chain 200 to take funds out of a SpokePool