feat: support erc1271 for verifying relay fee update sigs #221
Conversation
ACX-584 Add support for EIP1271
TLDR: Opportunity to contribute smart contract code to upgraded spoke pool. I think could be a very good candidate for someone who wants more exposure to smart contracts in an isolated low-risk issue. We ideally would like to support EIP1271 (https://eips.ethereum.org/EIPS/eip-1271) in the SpokePool. This would allow Gnosis Safe’s, smart wallets, and any contract to send speed up signatures. This would require the depositing contract to implement a function I think the simplest way to do this would be extend the SpokePool’s This could be collaborated on by two people or could just be one DRI. Whoever wants to do it we could do a pair programming session later this week to start work on it (edited) |
dohaki
requested review from
amateima,
james-a-morris,
mrice32 and
nicholaspai
| await testUpdatedFeeSignaling(depositor.address); | ||
| }); | ||
| it("EIP1271 - Can signal to relayer to use updated fee", async function () { | ||
| await testUpdatedFeeSignaling(erc1271.address); |
| await testUpdatedFeeSignatureFailCases(depositor.address); | ||
| }); | ||
| it("EIP1271 - Updating relayer fee signature verification failure cases", async function () { | ||
| await testUpdatedFeeSignatureFailCases(erc1271.address); |
There was a problem hiding this comment.
Can you add a test case for the erc1271 contract in particular that tries to re-enter the SpokePool via a malicious isValidSignature function? This way we can confirm that the SpokePool is protected against reentrancy via this avenue
There was a problem hiding this comment.
Hm actually maybe we should add a comment that reentrancy might be impossible:
"6e69fdc#diff-c828c513da79ba8aaef90514612391ab755989974a14d8e480ae02ebd9cfb4beL683"
There was a problem hiding this comment.
Since isValidSignature is a view method, re-entrancy can happen, but it cannot affect state.
nicholaspai
left a comment
nicholaspai
left a comment
There was a problem hiding this comment.
This is really well done, +1 on using OZ's existing library
| // like Argent are not supported) because of the possibility that a multisig that signed a message on the origin | ||
| // chain does not have a parallel on this destination chain. | ||
| require(depositor == ECDSA.recover(ethSignedMessageHash, depositorSignature), "invalid signature"); | ||
| bool isValid = SignatureChecker.isValidSignatureNow(depositor, ethSignedMessageHash, depositorSignature); |
There was a problem hiding this comment.
Maybe we should also add a comment about how isValidSignatureNow can change its behavior from block N to block M: https://github.com/OpenZeppelin/openzeppelin-contracts/blob/master/contracts/utils/cryptography/SignatureChecker.sol#L21
As long as the signature is valid at the time of calling fillRelayWithUpdatedFee then the signature is treated as valid.
There was a problem hiding this comment.
IMO, we should note that:
- EIP-1271 signatures are supported. This means that a signature valid now, may not be valid later.
- For an EIP-1271 signature to work, the depositor contract address must map to a deployed contract on the destination chain that can validate the signature.
- Regular signatures from an EOA are also supported.
There was a problem hiding this comment.
These are all great comments to add!
| // like Argent are not supported) because of the possibility that a multisig that signed a message on the origin | ||
| // chain does not have a parallel on this destination chain. | ||
| require(depositor == ECDSA.recover(ethSignedMessageHash, depositorSignature), "invalid signature"); | ||
| bool isValid = SignatureChecker.isValidSignatureNow(depositor, ethSignedMessageHash, depositorSignature); |
There was a problem hiding this comment.
IMO, we should note that:
- EIP-1271 signatures are supported. This means that a signature valid now, may not be valid later.
- For an EIP-1271 signature to work, the depositor contract address must map to a deployed contract on the destination chain that can validate the signature.
- Regular signatures from an EOA are also supported.
| await testUpdatedFeeSignatureFailCases(depositor.address); | ||
| }); | ||
| it("EIP1271 - Updating relayer fee signature verification failure cases", async function () { | ||
| await testUpdatedFeeSignatureFailCases(erc1271.address); |
There was a problem hiding this comment.
Since isValidSignature is a view method, re-entrancy can happen, but it cannot affect state.
Signed-off-by: Dong-Ha Kim <dongha.kim210@gmail.com>
Signed-off-by: Dong-Ha Kim <dongha.kim210@gmail.com>
dohaki
force-pushed
the
erc-1271-support
branch
from
382c16e to
15cdaf6
Compare
Closes ACX-584
As noted by @nicholaspai: This does open the possibility that someone could forge an EIP1271 signature and set the relayer fee to 50%, and then relay the honest depositor's deposit, stealing ~50% of the total deposit. We should comment on this and debate whether this risk is worth it