feat(spoke-pool): Allow relayer and depositor to agree to update relay fee #27

nicholaspai · 2022-02-08T03:53:19Z

No description provided.

mrice32

Looks awesome! A few minor questions/comments

mrice32 · 2022-02-08T07:04:51Z

contracts/SpokePool.sol

+        require(relayFills[relayHash] < totalRelayAmount, "filled relay");
+
+        // Depositor should have signed a hash of the relayer fee % to update to.
+        bytes32 expectedDepositorMessageHash = keccak256(abi.encode(newRelayerFeePct));


I think we need more than this to avoid replay attacks. For instance, if I agree to a relayer fee of 23% on a small relay, then I send a large relay, someone could take my signed message from the small relay and replay it to get a large fee from me in the in the large relay.

A few additional fields I think we need: depositId, chainId. We might also want include a constant value in front, like "ACROSS-V2 FEE 1.0" or something that we could increment if we ever updated spoke pools and ended up resetting the depositId counter.

ah good point these are easy to include

mrice32 · 2022-02-08T07:08:20Z

contracts/SpokePool.sol

@@ -268,6 +272,61 @@ abstract contract SpokePool is Testable, Lockable, MultiCaller {
        );
    }

+    function increaseRelayFee(


Should the relayer be able to use this method to actually fill the relay? If not, how does it get connected with a particular fill? Or is the idea that we would swap out the relayerFeePct for every FilledRelay that was submitted afterward?

The relayer in this implementation would call this method separately and it would not add to the fill. We could add an additonal method increaseRelayFeeAndFillRelay (bad name) that would call fillRelay AND modify the fee WDYT?

relayer in this implementation would call this method separately and it would not add to the fill

How would this work? Wouldn't the subsequent fill still be using the old relayer fee (since that's the only one that would match the hash)? So in that case, too much money would be sent to the recipient, no?

We don't have to do it this way, but the way I had conceptualized this was that we'd have a fill method that's identical to the existing one with two additional args: bytes memory signature, uint64 updatedRelayerFeePct. The signature would be verified against the provided fee, and the updated fee fee would be emitted in the FilledRelay event (we could also include a second event to signify that this fee was overridden or something).

So the relay hash would stay the same, with the old relayer fee, but the event would just change? (or have an additional one emitted)

I agree wth you that fillRelay could be overridden with a separate interface

Yeah, in the solution I had been thinking of, the relay hash/data would always be the same, the signature would be provided essentially as an override to the originally-set relayer fee.

So the event would be different and the amount sent to the recipient would be different (i.e. the relayFeePct used throughout the method).

interesting let me try this

ye, what @mrice32 describes fits with my understanding of this method. you can actually just overload the fillRelay method with an extra param which optionally lets you change the originally provided relayFeePct.

The only problem with overloading is that overloading doesn't play nicely with ethers like it does with web3. Do you have a strong opinion about overloading versus creating a new function name?

mrice32

#27 (comment)

…with updating fee

mrice32

Looks good! Minor comments

mrice32 · 2022-02-08T21:45:16Z

contracts/SpokePool.sol

+        bytes depositorSignature;
+    }
+
+    function fillRelayUpdatedFee(


nit: since this method is public, should it be moved up next to the fillRelay?

mrice32 · 2022-02-08T21:56:21Z

contracts/SpokePool.sol

+            bytes32 expectedDepositorMessageHash = keccak256(
+                abi.encode("ACROSS-V2-FEE-1.0", updatedRelayerFeeData.newRelayerFeePct, depositId, originChainId)
+            );
+            require(expectedDepositorMessageHash == updatedRelayerFeeData.depositorMessageHash, "incorrect new fee");


Why do we need them to pass in the message hash? Aren't we already generating it from other data they've provided?

mrice32 · 2022-02-08T21:56:34Z

contracts/SpokePool.sol

+            relayAmount: totalRelayAmount
+        });
+        bytes32 relayHash = _getRelayHash(relayData);
+        _fillRelay(relayHash, relayData, updatedRelayerFeeData.newRelayerFeePct, maxTokensToSend, repaymentChain);


mrice32 · 2022-02-08T21:57:07Z

contracts/SpokePool.sol

+        uint256 repaymentChain;
+    }
+
+    function _emitFillRelay(FillRelayEventData memory eventData) internal {


Is this extra method here just to avoid stack too deep errors?

chrismaree · 2022-02-08T22:14:37Z

contracts/SpokePool.sol

+
+    function _fillRelay(
+        bytes32 relayHash,
+        RelayData memory relayData,


is there a gas difference between this being memory and calldata?

according to this answer, calldata is cheaper.

Also aside, when testing I noticed that declaring a variable as memory does not contribute to the "stack too deep error", while calldata does?

Note that calldata is only avaialble for external methods. source

chrismaree · 2022-02-08T22:15:04Z

contracts/SpokePool.sol

+        _fillRelay(relayHash, relayData, relayerFeePct, maxTokensToSend, repaymentChain);
+    }
+
+    function _fillRelay(


do you have a section for internal methods? should probs be located there.

chrismaree · 2022-02-08T22:16:35Z

contracts/SpokePool.sol

+    ) internal {
+        // We limit the relay fees to prevent the user spending all their funds on fees.
+        require(
+            updatableRelayerFeePct <= 0.5e18 &&


I think one of these checks is redundant. if you validate relayData.realizedLpFeePct and (updatableRelayerFeePct + relayData.realizedLpFeePct) < 1e18 then it's impossible for updatableRelayerFeePct to be >0.5e18.

what if realizedLpFeePct = 0 but updatableRelayerFeePct = 0.6?

then just check that: updatableRelayerFeePct <= 0.5e18 && relayData.realizedLpFeePct <= 0.5e18?

chrismaree · 2022-02-08T22:17:43Z

contracts/SpokePool.sol

-            relayData.recipient
+        // Needed to resolve stack too deep compile-time error
+        _emitFillRelay(
+            FillRelayEventData(relayHash, relayData, updatableRelayerFeePct, fillAmountPreFees, repaymentChain)


dont emit RelayData. we dont want to emit structs. ideally what you had before was better so etherscan can parse it.

I take that back. your method you call does this 🙏

haha yep this is just unreadable to get around stack too deep

…art-contracts-v2 into npai/speed-up

mrice32

LGTM!

mrice32 · 2022-02-09T00:39:55Z

contracts/SpokePool.sol

+            // above keccak156 hash), then this will revert.
+            bytes32 ethSignedMessageHash = ECDSA.toEthSignedMessageHash(expectedDepositorMessageHash);
+            require(
+                SignatureChecker.isValidSignatureNow(depositor, ethSignedMessageHash, depositorSignature),


Minor thing we should be aware of: since this allows for ERC1271 "contract signatures", this can make a call out to the caller under the hood. Shouldn't be a problem, just wanted to mention it.

Hmmm thats a good point. As long as we reentrancy guard this method it should be ok though? I suppose also the calling contrct could call this contract back into fillRelay which I don't think could cause issues, but we could easily remedy by moving all this signature verification logic AFTER _fillRelay. This would also be more consistent perhaps with Check-effects-interaction pattern. WDYT?

Reading more into the method, I really don't think this is a problem, and it's arguable that it's even an interaction, because it makes a staticcall (https://github.com/OpenZeppelin/openzeppelin-contracts/blob/63b466901fb015538913f811c5112a2775042177/contracts/utils/cryptography/SignatureChecker.sol#L35), meaning no state can be modified anywhere inside that call (and subcalls). It's more akin to making a balanceOf call, which I think is considered a check rather than an interaction.

I think this is safe.

nicholaspai · 2022-02-09T04:20:42Z

Remaining comment here: #27 (comment)

@chrismaree wdyt?

mrice32

LGTM!

mrice32 · 2022-02-09T15:15:26Z

contracts/SpokePool.sol

+            // above keccak156 hash), then this will revert.
+            bytes32 ethSignedMessageHash = ECDSA.toEthSignedMessageHash(expectedDepositorMessageHash);
+            require(
+                SignatureChecker.isValidSignatureNow(depositor, ethSignedMessageHash, depositorSignature),


Reading more into the method, I really don't think this is a problem, and it's arguable that it's even an interaction, because it makes a staticcall (https://github.com/OpenZeppelin/openzeppelin-contracts/blob/63b466901fb015538913f811c5112a2775042177/contracts/utils/cryptography/SignatureChecker.sol#L35), meaning no state can be modified anywhere inside that call (and subcalls). It's more akin to making a balanceOf call, which I think is considered a check rather than an interaction.

I think this is safe.

…gnatures (#27) Co-authored-by: nicholaspai <[email protected]>

nicholaspai added 2 commits February 7, 2022 19:50

feat: can increase relay fee

799d438

Rename to IncreasedRelayFee

80b080e

nicholaspai requested review from mrice32 and chrismaree February 8, 2022 03:53

mrice32 reviewed Feb 8, 2022

View reviewed changes

nicholaspai requested a review from mrice32 February 8, 2022 17:51

mrice32 reviewed Feb 8, 2022

View reviewed changes

nicholaspai requested a review from mrice32 February 8, 2022 18:46

add more data to update relayer fee message hash and combine filling …

abe1ca9

…with updating fee

mrice32 reviewed Feb 8, 2022

View reviewed changes

chrismaree reviewed Feb 8, 2022

View reviewed changes

nicholaspai added 4 commits February 8, 2022 15:10

Merge branch 'master' of https://github.com/across-protocol/across-sm…

27a67ec

…art-contracts-v2 into npai/speed-up

reorganize methods

ec2cfa9

No need for relayer to pass in message hash

44ac914

Update SpokePool.Relay.ts

2cd3bbd

nicholaspai requested review from mrice32 and chrismaree February 8, 2022 23:41

mrice32 approved these changes Feb 9, 2022

View reviewed changes

nicholaspai changed the title ~~feat(spoke-pool): Allow relayer and depositor to agree to increase relay fee~~ feat(spoke-pool): Allow relayer and depositor to agree to update relay fee Feb 9, 2022

nicholaspai requested a review from mrice32 February 9, 2022 04:14

rename to "fillRelayWithUpdatedFee"

0808820

mrice32 approved these changes Feb 9, 2022

View reviewed changes

respnd to chris + matts comments

da5cc18

nicholaspai merged commit 25ff460 into master Feb 9, 2022

nicholaspai deleted the npai/speed-up branch February 9, 2022 19:57

pxrl added a commit that referenced this pull request Feb 20, 2024

improve: Restore TokensBridged and ExecutedRelayerRefundLeaf event si…

00b66f8

…gnatures (#27) Co-authored-by: nicholaspai <[email protected]>

feat(spoke-pool): Allow relayer and depositor to agree to update relay fee #27

feat(spoke-pool): Allow relayer and depositor to agree to update relay fee #27

Conversation

nicholaspai commented Feb 8, 2022

mrice32 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mrice32 Feb 8, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mrice32 left a comment

Choose a reason for hiding this comment

mrice32 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mrice32 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mrice32 Feb 9, 2022 • edited Loading

Choose a reason for hiding this comment

nicholaspai commented Feb 9, 2022

mrice32 left a comment

Choose a reason for hiding this comment

mrice32 Feb 9, 2022 • edited Loading

Choose a reason for hiding this comment

mrice32 Feb 8, 2022 •

edited

Loading

mrice32 Feb 9, 2022 •

edited

Loading

mrice32 Feb 9, 2022 •

edited

Loading