Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

feat(SpokePoolPeriphery): Support multiple exchanges #777

Open
wants to merge 36 commits into
base: master
Choose a base branch
from

Conversation

nicholaspai
Copy link
Member

@nicholaspai nicholaspai commented Nov 27, 2024

Currently we can only initialize the periphery contract with a single exchange to swap with. This PR allows us to initialize it with multiple exchanges to swap with. Like before, these initial set of exchanges and function selectors cannot be changed post-initialization, which gives the user assurances.

This also adds a method whitelistExchanges which can update the whitelisted exchange list. This means that the contract is now Ownable and both initialize and whitelistExchanges are marked onlyOwner. This means that users need to trust the owner of the contract now, and the worst thing that the owner can do is whitelist a malicious exchange. The caller of the swapX functions would still need to knowingly sign off on routerCalldata that would target this malicious exchange, so the user still retains control

I don't think we're going to make the contract Ownable and let the owner update the whitelisted exchange list post-deployment, but if we want to we can revert this commit. The reasoning is to give better assurances to the user about the contract's expected behavior.

This PR removes the whitelist of exchanges that can be called by the SpokePoolPeriphery, which was useful for protecting the caller from approve-ing the SpokePoolPeriphery contract and leaving an approval that could be stolen by another user who could direct the SpokePoolPeriphery to transferFrom the user who left their approval hanging. The reasoning for removing the whitelist is so that multiple exchanges can be used by the caller. In order to keep protecting the user from this "approval abuse", we add a new "Proxy" contract that should be used by the caller to call functions like SpokePoolPeriphery.swapAndBridge that require approvals. Now the user only approves the proxy contract which can only be used to call SpokePoolPeriphery.swapAndBridge rather than execute any arbitrary calldata.

Currently we can only initialize the periphery contract with a single exchange to swap with. This PR allows us to initialize it with multiple exchanges to swap with. Like before, these initial set of exchanges and function selectors cannot be changed post-initialization, which gives the user assurances.
@nicholaspai nicholaspai marked this pull request as ready for review November 27, 2024 13:51
@nicholaspai
Copy link
Member Author

I just added some forge unit tests for:

  • swapAndBridge
  • initialize
  • deposit

@nicholaspai nicholaspai requested a review from bmzig November 27, 2024 19:53
Make user approve proxy contract so no one can use `exchange` + `routerCalldata` to steal their already approved funds via the `SpokePoolPeriphery`
@nicholaspai nicholaspai requested a review from bmzig December 1, 2024 21:55
@nicholaspai nicholaspai added the do not merge do not merge label Dec 5, 2024
* feat: add permit2 entrypoints to the periphery

Signed-off-by: Bennett <[email protected]>

* Update test/evm/foundry/local/SpokePoolPeriphery.t.sol

* Update SpokePoolPeriphery.t.sol

* move permit2 to proxy

* fix permit2

Signed-off-by: bennett <[email protected]>

* wip: swap arguments refactor

Signed-off-by: bennett <[email protected]>

* implement isValidSignature

Signed-off-by: bennett <[email protected]>

* 1271

Signed-off-by: bennett <[email protected]>

* simplify isValidSignature

Signed-off-by: bennett <[email protected]>

* rebase /programs on master

Signed-off-by: nicholaspai <[email protected]>

* clean up comments

* rebase programs

* fix: consolidate structs so that permit2 witnesses cover inputs

Signed-off-by: bennett <[email protected]>

* begin permit2 unit tests

Signed-off-by: bennett <[email protected]>

* rebase

* Update SpokePoolPeriphery.t.sol

* move type definitions to interface

Signed-off-by: bennett <[email protected]>

* fix permit2 test

Signed-off-by: bennett <[email protected]>

* transfer type tests

Signed-off-by: bennett <[email protected]>

* rename EIP1271Signature to Permi2Approval

Signed-off-by: bennett <[email protected]>

---------

Signed-off-by: Bennett <[email protected]>
Signed-off-by: bennett <[email protected]>
Signed-off-by: nicholaspai <[email protected]>
Co-authored-by: nicholaspai <[email protected]>
Co-authored-by: nicholaspai <[email protected]>
@nicholaspai nicholaspai requested a review from bmzig December 6, 2024 23:34
bmzig
bmzig previously approved these changes Dec 17, 2024
Copy link
Contributor

@bmzig bmzig left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is a really good improvement. Another thing I was wondering was if we could not pass in a struct to _swapAndDeposit but IIRC that causes a stack too deep error.

bmzig and others added 3 commits December 18, 2024 14:57
* feat: add permit2 entrypoints to the periphery

Signed-off-by: Bennett <[email protected]>

* Update test/evm/foundry/local/SpokePoolPeriphery.t.sol

* Update SpokePoolPeriphery.t.sol

* move permit2 to proxy

* fix permit2

Signed-off-by: bennett <[email protected]>

* wip: swap arguments refactor

Signed-off-by: bennett <[email protected]>

* implement isValidSignature

Signed-off-by: bennett <[email protected]>

* 1271

Signed-off-by: bennett <[email protected]>

* simplify isValidSignature

Signed-off-by: bennett <[email protected]>

* rebase /programs on master

Signed-off-by: nicholaspai <[email protected]>

* clean up comments

* rebase programs

* feat: sponsored swap and deposits

Signed-off-by: bennett <[email protected]>

* fix: consolidate structs so that permit2 witnesses cover inputs

Signed-off-by: bennett <[email protected]>

* begin permit2 unit tests

Signed-off-by: bennett <[email protected]>

* rebase

* Update SpokePoolPeriphery.t.sol

* move type definitions to interface

Signed-off-by: bennett <[email protected]>

* fix permit2 test

Signed-off-by: bennett <[email protected]>

* transfer type tests

Signed-off-by: bennett <[email protected]>

* rename EIP1271Signature to Permi2Approval

Signed-off-by: bennett <[email protected]>

* add mockERC20 which implements permit/receiveWithAuthorization

Signed-off-by: bennett <[email protected]>

* add tests for permit, permit2, and receiveWithAuth swaps/deposits

Signed-off-by: bennett <[email protected]>

* add tests for invalid witnesses

Signed-off-by: bennett <[email protected]>

* factor out signature checking

Signed-off-by: bennett <[email protected]>

---------

Signed-off-by: Bennett <[email protected]>
Signed-off-by: bennett <[email protected]>
Signed-off-by: nicholaspai <[email protected]>
Co-authored-by: nicholaspai <[email protected]>
Co-authored-by: nicholaspai <[email protected]>
)

* feat: add permit2 entrypoints to the periphery

Signed-off-by: Bennett <[email protected]>

* Update test/evm/foundry/local/SpokePoolPeriphery.t.sol

* Update SpokePoolPeriphery.t.sol

* move permit2 to proxy

* fix permit2

Signed-off-by: bennett <[email protected]>

* wip: swap arguments refactor

Signed-off-by: bennett <[email protected]>

* implement isValidSignature

Signed-off-by: bennett <[email protected]>

* 1271

Signed-off-by: bennett <[email protected]>

* simplify isValidSignature

Signed-off-by: bennett <[email protected]>

* rebase /programs on master

Signed-off-by: nicholaspai <[email protected]>

* clean up comments

* rebase programs

* feat: sponsored swap and deposits

Signed-off-by: bennett <[email protected]>

* fix: consolidate structs so that permit2 witnesses cover inputs

Signed-off-by: bennett <[email protected]>

* begin permit2 unit tests

Signed-off-by: bennett <[email protected]>

* rebase

* Update SpokePoolPeriphery.t.sol

* move type definitions to interface

Signed-off-by: bennett <[email protected]>

* fix permit2 test

Signed-off-by: bennett <[email protected]>

* transfer type tests

Signed-off-by: bennett <[email protected]>

* rename EIP1271Signature to Permi2Approval

Signed-off-by: bennett <[email protected]>

* add mockERC20 which implements permit/receiveWithAuthorization

Signed-off-by: bennett <[email protected]>

* add tests for permit, permit2, and receiveWithAuth swaps/deposits

Signed-off-by: bennett <[email protected]>

* add tests for invalid witnesses

Signed-off-by: bennett <[email protected]>

* feat: Delete SwapAndBridge and add submission fees to gasless flow

SwapAndBridge is to be replaced with SpokePoolV3Periphery

Gasless flows will require user to cover gas cost of whoever submits the transaction, but they can be set to 0 if the user wants to submit themselves.

* Internal refactor

* Update SpokePoolV3Periphery.sol

* Update PeripherySigningLib.sol

* Update SpokePoolV3Periphery.sol

* Update PeripherySigningLib.sol

---------

Signed-off-by: Bennett <[email protected]>
Signed-off-by: bennett <[email protected]>
Signed-off-by: nicholaspai <[email protected]>
Co-authored-by: Bennett <[email protected]>
@nicholaspai
Copy link
Member Author

_swapAndDeposit

Does this function exist anymore?

@@ -155,7 +230,7 @@ contract SpokePoolV3Periphery is Lockable, MultiCaller {
uint32 fillDeadline,
uint32 exclusivityParameter,
bytes memory message
) external payable nonReentrant {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not directly related to this specific diff, but is there a reason why you can not specify an explicit outputToken here?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is a copy/paste of the spoke pool verifier deposit function, so we just assume that this is only being used with the native token,

@nicholaspai
Copy link
Member Author

nicholaspai commented Mar 3, 2025

I think we need to re-review this PR given the possibility of merge conflicts since it was last "finalized"

Last state of repo pre the large merge with master: https://github.com/across-protocol/contracts/tree/c1f618173eace1dd03ea2a80124c07264e473840

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants