ERC-4626 Support

[Joeysantoro]

Given the “universal” goal of the router, it would be appropriate to consider including a number of instructions for ERC-4626 standard compatibility

This unlocks use cases where users can swap their tokens into and out of a desired yield bearing protocol, essentially covering the majority of remaining use cases

[Joeysantoro]

https://github.com/fei-protocol/ERC4626/blob/main/src/ERC4626Router.sol was intended to achieve this goal specifically focused on the erc4626 component, modeled after the Uni v3 router. Combining the functionality with Universal Router would multiply the available use cases.

I’m happy to make a PR if there is interest

[marktoda]

Hey Joey! Great idea, we'd love to have ERC-4626 support in Universal Router! I think it just slipped our mind when speccing out the commands. Feel free to make a PR and we'll check it out :D

[Joeysantoro]

Thanks @marktoda! Starting at the highest level architecture, I'd assume the right way to go is to

1. add a module similar to the Uni v2 and v3 ones with support for the 4 mutable ERC4626 methods + a slippage check. Basically copying the logic from ERC4626RouterBase

2. add a dispatch elif section for these commands

3. Afterwards, work on integrating into sdk

I'm noticing that "Maximum supported command at this moment is 0x1F." which means there is only support for 5 more commands and they are not contiguous. What is the suggestion here?

The other option is to finalize the ERC4626 Router as a standalone contract and add a single command for an external call to that router (less gas efficient and the work/auditing to get 4626 router in prod is nontrivial).

Lmk initial thoughts

[marktoda]

Hey, this plan sounds good to me. The impl in ERC4626RouterBase looks great.

Note you will need to include permit2 integration. I think ERC4626 uses only approve/transferFrom flow, right? So can't do payOrPermit2Transfer as we do in the uniswap module. I suppose need to do permit2TransferFrom(token, msg.sender, address(this), amount) to take into the router first, and then approve the erc4626 token.

Wrt the commands, this got a bit more complicated in a recent commit where we added sub-branching for the command dispatcher to decrease #lookups in the general case. Previously was just a long linear if-tree. IIRC the discontinuity is to balance the tree to get good average case lookup gas.. So yeah, you could interweave the ERC4626 commands into the open slots (looks like we have >4 available). Potentially cleaner though to just add a new branch (0x20-0x28). Will defer to @hensha256 on this part

[marktoda]

I suppose need to do permit2TransferFrom(token, msg.sender, address(this), amount) to take into the router first, and then approve the erc4626 token

Could also omit this and require it be included as a pre-command as well. Some gas overhead to consider here. Though this also limits ability to get the inputs for ERC4626 from another previous command ie a swap

Maybe best middleground is something like what we do here

universal-router/contracts/modules/uniswap/v2/V2SwapRouter.sol

Line 62 in 5e237de

if (

[Joeysantoro]

Potentially cleaner though to just add a new branch (0x20-0x28). Will defer to @hensha256 on this part

How hard would it be to do this? I think it would be best to add 2 additional commands on top of the base 4, namely depositMax and redeemMax. These would also partially solve the issue of dynamically getting the balance from a previous command

From what I can tell on the contract side all I'd have to do is increase the COMMAND_TYPE_MASK to 0x3f, and add additional branch masks and logic. Not sure if things will break on the sdk or whatnot.

[Joeysantoro]

I also found one issue, because ERC-4626 uses the approve/transferFrom flow, the ERC-4626 example router impl has a generic approve function which is public: https://github.com/fei-protocol/ERC4626/blob/main/src/external/PeripheryPayments.sol#L30-L32

This is not a security issue in that instance because the router is meant to not hold any tokens and the only state it holds are approvals. It saves gas across calls because the router can maintain infinite approvals to different vaults after the first call.

It seems that is also true of the universal router, but I wouldn't want to add such a command without more discussions.

If it can't be added, then each deposit/mint call will need to approve beforehand which will increase the gas, but may be necessary from a security perspective.

Unless there is more consensus around adding an approve command, perhaps the best route would be to always approve in deposit/mint calls for the next version of the router, and consider an approve function to memoize gas in a future version

[marktoda]

It seems to me that a global approve should be safe. As you say the router never holds tokens and only uses user-approvals when initiated by that user. @hensha256 and @ewilz to confirm / double check

[ewilz]

we have an extra 2 bits available to accommodate larger commands (README has some details). I think that would be cleaner than using random open slots, @hensha256 would know better, but filling in certain bits could trigger some of our flags. I actually think the gas savings for the nested logic doesn't justify how complicated it makes the code, I'd be very down to take it out, but maybe I can make another issue for that :P

[hensha256]

Hey friends! Sorry for the late response here I took a little time off after the announcement!

And hey @Joeysantoro ! Thanks so much for contributing to our project 👯

Yes we currently have only 5 commands spare with the 0x1f flag - i've made PR #183 to try to make it much clearer which commands are still open as it felt like a confusing setup before. However yes if what would be best here is 6 commands we can definitely open up the router to having an extra bit for the commands!

increase the COMMAND_TYPE_MASK to 0x3f

yep thats all that would be needed on the contract side. I think that should be easy enough and then we can figure out exactly here to put the 4626 commands after that!

[hensha256]

With respect to the approve logic we actually had a PR in to do so #117 that we decided not to merge for now. I can't currently think of a security issue with adding in such an approval command (the reason we didnt merge it was that we didnt need it yet) but it definitely opens up more avenues of attack that I would want to think through...

Definitely interested to see a PR for your ideal setup including approval command as I feel that will be easier to review and think through attack vectors!

[Joeysantoro]

Thanks for the feedback all! I believe I have enough info to get started on a PR, however I wanted to prioritize the conversation from the permit2 side Uniswap/permit2#162 as I believe that is a bit more time sensitive if I understand correctly

The integration with the router would also change somewhat depending on whether and where a permit2 integration exists