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

Circle CCTP in FiberRouter's Foundry Swap & Withdraw Functions #48

Merged
merged 8 commits into from
May 1, 2024

Conversation

salmanferrum
Copy link
Contributor

@salmanferrum salmanferrum commented Apr 16, 2024

PR Description

Summary:
This pull request implements enhancements to FiberRouter's swap and withdraw functions to integrate Circle CCTP (Cross-Chain Transfer Protocol) support. The changes introduce a new boolean parameter cctpType and integrate the CCTPFundManager contract for handling CCTP transactions efficiently. This integration enables FiberRouter to facilitate higher-volume asset transfers between source and target networks, enhancing the MultiSwap functionality and cross-chain liquidity.

Changes:

  1. _cctpSwap Internal Function:

    • An internal function is introduced to perform the cctp swaps by calling the depositForBurn function of CCTP.
    • This function allows the burn of tokens on source network and deposit of tokens on target network's CCTPFundManager.
  2. New Boolean Parameter cctpType:

    • Added a new boolean parameter cctpType to the swap and withdraw functions in FiberRouter.
    • This parameter distinguishes between standard asset transfers and CCTP-enabled asset transfers.
  3. Integration of CCTPFundManager Contract:

    • Introduced the CCTPFundManager contract to manage the receiving of CCTP funds from the source network.
    • Integrated FiberRouter with the CCTPFundManager contract for seamless handling of CCTP transactions.

Please Note:
Circle's USDC Address on arbitrum is different from our foundry USDC. As the USDC we are using is from centre.io however, Circle USDC needs to be used as a FoundryAsset on Arbitrum.

* @notice This constructor is called only once during the deployment of the contract.
*/
constructor() EIP712(NAME, VERSION) {
bytes memory initData = IFerrumDeployer(msg.sender).initData();
Copy link

Choose a reason for hiding this comment

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

If there is no initialization, not needed

@param _fiberRouter is the FiberRouter address
*/
function setFiberRouter(address _fiberRouter) external onlyOwner {
fiberRouter = _fiberRouter;
Copy link

Choose a reason for hiding this comment

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

Better to also initialize in constructor

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sure thing

require(amount != 0, "FR: Amount must be greater than zero");
require(salt > bytes32(0), "FR: Salt must be greater than zero bytes");

if (cctpType) {
Copy link

Choose a reason for hiding this comment

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

Isn't it better to create a separate method for cctp wd? In what case cctp wd is used instead of normal wd?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Cctp withdrawal is done when we have we need to withdraw funds from cctp fund manager to perform a bigger amount of usdc swap such as 50K USDC. When we perform cctp swap, we deposit our usdc funds to target cctp fund manager using cctp and on target network, we can call cctpFundManager's withdraw function to get the amount moved to target user address.

We can create separate functions but that will increase the size of contract as in both cases we are doing the same checks and actions. However, in one function all we need to do is to check off swap type (cctp type == true) then we can call cctp FundManager withdraw function otherwise normal flow will be performed.

@lukmaan-ferrum
Copy link
Contributor

Generally looks good to me but is there a reason for having a separate fund manager contract (CCTPFundManger) vs just using the existing FundManger contract?

Functions are the same and I can't tell why the same pool of funds can't be used for both cctp and non-cctp swaps

@salmanferrum
Copy link
Contributor Author

So here is the thing:

  1. Separate fund managers for cctp is because we have to use the usdc that we received from cctp protocol and these funds take time to reach to destination chain
  2. Our fund manager (pool) currently have a limited threshold of liquidity such as max value is 10K however, cctp is originally implemented to support hundred thousand $ worth of swaps which is why we use cctp FundManager to keep it separate for both chains
  3. Moreover, we need to have cctp fund manager on both ends so that on swap, we perform cctp swap of usdc (burn on source chain & deposit on target chain's cctp fund manager) and on withdraw, fetch the cctp usdcs (100K usdc) and then transfer it to fiber router to perform 1inch swap.

Moreover, I would request you to please review the security implications of transferring funds to fiber router and see if the balances are checked before making tx and if the sync pattern we are using should be adopted to fiber router as well?

@lukmaan-ferrum
Copy link
Contributor

  1. If the funds take time, I'm assuming we are waiting for Circle to deposit the USDC to our CCTPFundManager. If that's the case then I still don't get why a separate fund manager is needed
  2. Ah ok this one makes sense. What was the reason for setting this lower cap btw?
  3. Maybe I've misunderstood how CCTP works. I'm assuming Circle deposits the USDC to whichever contract we tell it (they submit tx from their side), and we don't need to 'pull' the funds. If that's the case, the general flow still looks the same to me as normal swaps e.g:
    • Money is taken in to our FundManger on source chain
    • Money is taken out from our FundManger on destination chain
      • If backend is waiting for Circle to deposit, then we can be sure that funds are there

I've just realised that maybe you are proposing we pre-fill the CCTPFundManger with a ton of USDC on all chains, and we take on some small risk by pre-emptively using our stockpile of USDC and swap to the user. We then hope that CCTP doesn't break and we get back our USDC that we're missing. If that's the case then ignore my comments for (1) and (3)

For security implications of asset transfer to FiberRouter, looks fine to me for a couple of reasons:

  • We're checking that require(FRBalance >= amountIn, "FR: FRBalance should be greater than the amountIn");
    I would say that we might not even need this check, because FRBalance will always be equal to amountIn, since this is what we pass in to CCTPFundManager(sourceCCTPFundManager).withdrawSignedOneInch(to, amountIn, ...)
    If that's the case then we're guaranteed to not have any leftover funds in the FiberRouter after the swap on oneInch/some other aggregator is done.
  • I have some additional checks in my PR which makes sure user gets their money

Slightly related to the above, why do we use the sync pattern? I first thought its because of feeOnTransfer tokens, but since those would not be used as foundry tokens, then I'm not sure why we need it

@salmanferrum
Copy link
Contributor Author

So for your 1 & 3 comments,

  1. We need to deposit usdc from source network directly to target network's fund manager or cctp FundManager because cctp calls deposit for burn function, which simply burns the token on source chain and deposit it on target network's address

  2. We have had set the threshold for only first launch to see how it goes and in future we can set it to a bigger threshold

  3. The separate cctp fund manager is there because we need to keep usdc funds being transferred from cctp protocol and to differentiate. We can have this removed or shifted when we have an open these'd hold for liquidity management in FundManager

  4. Moreover there is a situation where we can transfer users fron FundManager directly to user on target chain to accommodate a speedy swap instead of waiting for cctp to transfer funds in 15 minutes and on other hand, after 15 minutes once funds are there we can accommodate this in our FundManager etc

And for your other questions, FRBalance check is needed because in fiber router, it's not necessary that only amount in is deposited in fiber but there can be a chance where someone transfers tokens in FiberRouter or in any case, if the previous swap fails where the tokens are kept in fiber router but next steps were not processed then in that case the previous balance of fiber Router plus the amount In will be different from the amount in and it will avoid the transfer of more tokens than the requested.

Lastly, sync pattern is used in FundManager to maintain the token inventory and keep it updated with fund manager's balance whenever there is a swap in our swap out.

@lukmaan-ferrum
Copy link
Contributor

Ok that all makes sense. Don't think sync pattern is needed in FiberRouter and current checks are enough to me, so will approve

Copy link
Contributor

@lukmaan-ferrum lukmaan-ferrum left a comment

Choose a reason for hiding this comment

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

Approved after some discussion

@taha-abbasi taha-abbasi merged commit a37aca0 into ferrumnet:develop May 1, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants