Fujicracy / fuji-v2

Cross-chain money market aggregator
https://fuji-v2-frontend.vercel.app
15 stars 10 forks source link

ConnextRouter bridging slippage #253

Closed 0xdcota closed 1 year ago

0xdcota commented 1 year ago

When bridging assets over through Connext, the bridged funds are exposed to slippage within the internal mechanics of Connext. The slippage can be positive or negative. Positive slippage means you can receive more amount of the asset as you intended to send across chains.

As described by the Connext team:

Connext has internal AMMs on both the origin and destination chains when you transfer assets. If you specify zero slippage, the transaction can fail either if:

(1) you exceed max slippage on the origin chain - in this case the transaction reverts immediately (2) the transaction goes through the AMM on the origin chain, but then hits max slippage on the destination chain In this case the transaction halts on the destination and you can either:

  • retry it later with the same slippage on the destination chain
  • update the slippage on the dest, or
  • exit into nextAsset on the dest. From there you could also send the nextAssets back to the origin or some other chain if you'd like.

Note that for (2) the protocol takes the sum of the slippage on both sides so you may have positive slippage on one chain that cancels out some slippage on the other chain.

0xdcota commented 1 year ago

Notes on xReceive considerations for slippage

xReceive( 
bytes32 transferId,
uint256 amount,
address asset,
address, /* originSender */
uint32 originDomain,
bytes memory callData
)

Assertions (from Connext call Jan-17-2023) • If asset == address(0), then amount must == 0. • At any time, amount >= 0

If amount == 0; This means receiving call is just calldata transfer.
Therefore, Fuji's possible first actions are:

No slippage value replacement is required in any of these actions.

If amount > 0; The receiving call is calldata and value transfer. Therefore, Fuji's possible first actions are:

Slippage value replacement should occur.

0xdcota commented 1 year ago

Brainstorm notes for the SDK building an xcall.

brozorec commented 1 year ago
  • In the argument slippage of an xcall(), we should not allow the slippage value above a predetermined threshold that should be predefined overall at protocol level. I propose this threshold to be 5 bps (%0.05). That way we can control and inform users that they should expect maximum 10 bps as "router fee + slippage" from their cross-chain tx.

Hmm I'm not sure that hardcoding a slippage is the most optimal way. What about adding another parameter for DEPOSIT, PAYBACK and SWAP actions that will be checked in ConnextRouter._accountForSlippage? What would be the impact of such a change?

  • Do not allow near liquidation deposit-and-borrow xcall operation. Connext-bridge-slippage unbounded (opposed to described in the previous point) + router fee may result in a situation of immediate liquidation if the user inputs near max LTV borrowing position. We should avoid such possibilities at least from "ordinary-front-end" user interactions.

Agree with this. Added https://github.com/Fujicracy/fuji-v2/issues/242#issuecomment-1396776652

0xdcota commented 1 year ago

Hmm I'm not sure that hardcoding a slippage is the most optimal way. What about adding another parameter for DEPOSIT, PAYBACK and SWAP actions that will be checked in ConnextRouter._accountForSlippage? What would be the impact of such a change?

The bridge-slippage in this case is particular to Connext mechanics. I will then prefer that we keep this aspect of the logic at the initial data decoding (shown below) vs doing it at the action level. https://github.com/Fujicracy/fuji-v2/blob/cb9df6289fa39b7b74906b59bdfce81ee02288dc/packages/protocol/src/routers/ConnextRouter.sol#L111 This way the internalBundle doesn't have to handle this parameter if not used in other type of bridge implementations. This alternative has less impact at SC level, but will require SDK to add the extra parameter.

brozorec commented 1 year ago

@DaigaroCota We add a third param to decode, right? If so, sounds good to me :+1: