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

Fix Sandwich attack in Router's crosstransfer function #484

Conversation

pedrovalido
Copy link
Contributor

This PR addresses H-6

@pedrovalido pedrovalido added Bug Something isn't working Smart Contracts labels Apr 21, 2023
@pedrovalido pedrovalido self-assigned this Apr 21, 2023
@pedrovalido pedrovalido linked an issue Apr 21, 2023 that may be closed by this pull request
@pedrovalido pedrovalido changed the base branch from main to protocol/fix/macro-findings-critical-high April 21, 2023 11:59
@0xdcota 0xdcota marked this pull request as ready for review May 4, 2023 16:26
@0xdcota 0xdcota self-requested a review May 4, 2023 16:47
@0xdcota 0xdcota merged commit 9bc3adf into protocol/fix/macro-findings-critical-high May 4, 2023
@0xdcota 0xdcota deleted the protocol/fix/crosstransfer-sandwich-attack branch May 4, 2023 16:48
@0xcuriousapple
Copy link
Collaborator

0xcuriousapple commented May 16, 2023

hey @daigarocota
as per as I understand you are accepting delegate as a param now and have replaced it from msg.sender.

if yes
can you please explain how it solves the concerned issue?
can't the caller pass themselves as delegate only in params?

@0xdcota
Copy link
Contributor

0xdcota commented May 17, 2023

can you please explain how it solves the concerned issue?

The delegate is set by the user-caller who initiated the tx in the origin chain, and they are most likely the receiver of the assets on the destination chain. Previously, as you noted it was the msg.sender. We figured there were situations in where the msg.sender context would be the Connext address; specifically for a three chain operation.
Now, delegate as an arg, allows the user to be in control of the slippage all the way through the chain of txns.

can't the caller pass themselves as delegate only in params?

I am failing to find a way how a "malicious" caller, can sandwhich another user. Neither _crossTransfer, or _crossTransferWithCall will allow someone random to pull another user's funds. If the user for some reason sets a random address in control of their "delegate" slippage, I see it no different than approving erc20 to a random address.

@0xdcota
Copy link
Contributor

0xdcota commented May 18, 2023

hey @daigarocota as per as I understand you are accepting delegate as a param now and have replaced it from msg.sender.

if yes can you please explain how it solves the concerned issue? can't the caller pass themselves as delegate only in params?

Update was done.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Bug Something isn't working Smart Contracts
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Sandwich attack on _crossTransfer
3 participants