superfluid-finance / protocol-monorepo

Superfluid Protocol Monorepo: the specification, implementations, peripherals and development kits.
https://www.superfluid.finance
Other
874 stars 237 forks source link

[ETHEREUM-CONTRACTS] System of Approvals Spike #1482

Closed 0xdavinchee closed 3 months ago

0xdavinchee commented 1 year ago

A spike to investigate how we could implement a cohesive system of approvals and make it work across the board in the protocol, related to GDA:

More generally:

  1. Approval / delegation system for all core write functions
  2. No overhanging unused functionalities (i.e. operator)
  3. Flexible upgrade/downgrade (maybe the generic one with from and to which we discussed earlier)
  4. Explore if we want/need permit. It keeps coming up now as more ppl focus on UX

Things to consider

Idea was originally from Fran after a brief discussion of the from parameter in distribute and distributeFlow

hellwolf commented 1 year ago

Maybe we get rid of the operator?

yes. any time.

hellwolf commented 1 year ago

Approve maybe could work for distribute() as well ACL could work for CFA and GDA

XLarge t-shirt project, could be a next feature in the pipeline after GDA.

Dubbing it to "Unified ACL system".

Importance: Cherry-on-top & design surface improvements for the SuperToken. Perhaps medium. Can discuss in the future refinement meeting.

hellwolf commented 1 year ago

Maybe we explore permit/permit2 whatever new stuff exists

Continue to watch on the side line:

image.png

image.png

hellwolf commented 1 year ago

Flexible upgrade/downgrade (maybe the generic one with from and to which we discussed earlier)

Can revisit this, and piggyback a new SuperToken upgrade again if needed.

hellwolf commented 1 year ago

@vmichalik

d10r commented 1 year ago

Observation: When requesting a signature for token allowance using permit2, Metamask doesn't provide an option to adjust the amount. E.g. here it's triggered by the Uniswap App and the only option I have is to give unlimited allowance (more precisely: 2^160 - likely some Uniswap specific packing optimization). image

d10r commented 1 year ago

Dune Dashboard: https://dune.com/toda/permit2-stats

Ethereum

image

image

Polygon

image

image

kasparkallas commented 1 year ago

From my understanding, Permit2 would enable us to use a single on-chain transaction in all current user product scenarios if the Super Tokens and SuperApp-based peripheral contracts supported it. I don't count the Permit2 signing as a transaction. Otherwise the best we can usually do for EOA-s is 2 transactions (1 on-chain allowance transaction and 1 Super Token based batch transaction).

Links:

philipandersson commented 1 year ago

https://blog.uniswap.org/permit2-integration-guide

hellwolf commented 1 year ago

Overall, the adoption is steady, we should adopt permit2 in our frontend.

Techinical Notes

However there are still couple of different modes of permit transfers:

What About Wallet Experience?

Do we have special metamask UI view of the permit2? SCREENSHOT PLEASE

Tighter Permit2 Integration into Super Token Logic

...

Underlying Token Permit2 Support in Automation Contracts

...

PoCs

0xdavinchee commented 1 year ago

Notes taken from a further sync w/ @d10r, @kasparkallas and myself after Let's Discuss:

Permit2

Initial investigation objective: Investigate how integrating Permit2 will improve overall UX when using Superfluid.

Root issue: Improve overall UX when wrapping from erc20 => supertoken

conclusions

What do I want as the user?

possible solutions:

simple poc

end game ideas

resources

kasparkallas commented 1 year ago

A sketch to give an idea of a possible contract that would enable better UI/UX with increased security if this contract became the only source of underlying token allowance for user products: image

It tries to solve:

d10r commented 1 year ago

UX/security sweet spot

The simplest solution for maximizing UX would be: a) change the default amount for underlyingERC20.approve to max/unlimited. b) add SuperToken.upgradeFor which allows somebody (or even anybody) to upgrade underlying tokens to SuperTokens up to the allowance provided to the SuperToken contract on behalf of arbitrary SuperToken users.

The permission to trigger upgradeFor could be reserved to a dedicated contract which somehow always knows when a user wants more tokens to be automatically upgraded.
In theory the information flow necessary to achieve that could take place offchain. But there's an important caveat: this would expose the whole balance of underlyingERC20 to the smart contract risk and to the governance risk of the Superfluid framework, which is not always acceptable.

So, how much of this ideal solution (give approval only once) could we preserve without this full exposure?

Turns out, there's 2 hard constraints: a) the contract given ERC20.approval to shall not be exposed to SF gov (either not upgradable or more stringent upgrade constraints) b) the contract given unlimited approval to is self-constrained by a user-defined time-based allowance which is persisted such that SF gov can't change it

Such a contract - say UpgradeManager - could still give some privileged access to SF gov, e.g. allow it to set mappings from ERC20s to canonical SuperTokens. The worst case scenario here would be that a bad SF gov diverts underlyingERC20 to a rouge SuperToken, but the time based allowance limit would still apply, severely limiting the potential damage.

If the time-based allowance is stored in underlyingERC20 itself, a single transaction (invocation of underlyingERC20.approve) or signature (signed permit - be it native to underlyingERC20 or transitively through the Permit2 contract) could be sufficient for triggering a wrapping operation or even repeated wrapping / autowrap - assuming background automation.
This could be achieved by setting the amount not to type(uint256).max, but somehow additionally encode the time-based approval into that value, e.g. define that for a given range below that max value, the offset is interpreted as time based allowance. E.g. we could specify the last 96+48 bits to encode a maxUpgradeRate and startTs, leaving 112 bits (= ~5e15 tokens for tokens with 18 digits) for conventional allowance values. For any sane token, that amount is effectively just as unlimited as 2^256.

A possible implementation could look something like this:

contract UpgradeManager {
    mapping(address => uint256) consumedAllowances;

    function fetchTokens(IERC20 erc20, address holder, uint256 amount) external {
        if (msg.sender != getCanonicalSuperToken(erc20)) revert("msg.sender is not canonical SuperToken");

        // The allowance value stored in the underlying is interpreted as "permissions", encoding a start timestamp and a max upgrade rate
        uint256 permissions = erc20.allowance(holder, address(this));

        uint256 totalAllowanceNow = (block.timestamp - getStartTimestamp(permissions)) * uint256(uint96(getMaxUpgradeRate(permissions)));

        if (consumedAllowances[holder] + amount > totalAllowanceNow) {
            revert("insufficient remaining allowance");
        } else {
            consumedAllowances[holder] += amount;
            erc20.transferFrom(holder, msg.sender, amount);
        }
    }

    function getCanonicalSuperToken(IERC20 erc20) internal returns(address) {/*implementation...*/
    function getStartTimestamp(uint256 permissions) internal returns(uint48) {/*implementation...*/}
    function getMaxUpgradeRate(uint256 permissions) internal returns(int96) {/*implementation...*/}
}

contract SuperToken {
    function upgradeFor(address holder, uint256 amount) external {
        _upgradeManager.fetchTokens(_underlyingToken, holder, amount);
        // args: operator, account, to, amount, userData, operatorData
        _upgrade(msg.sender, address(_upgradeManager), holder, amount, "", "");
    }
}

Here, the ERC20 is intermittently owned by the UpgradeManager contract. By adding an variant of SuperToken._upgrade which doesn't itself invoke _underlyingToken.safeTransferFrom(), the UpgradeManager could instead do a transferFrom directly to the SuperToken contract.

Note that the limits additionally encoded into the allowance value can only add constraints. There's no technical possibility (and thus no risk) for those additional semantics to undermine the constraints encoded by default and enforced by the ERC20 contract itself.

interplay with permit

SuperToken.upgrade[To|For] could be extended (e.g. via overloading) to optionally accept a signed permit. UpgradeManager could then be made capable of using such a signed permit to fetch tokens either directly from an ERC20 with builtin permit support (as is the case e.g. for USDC) or through Permit2.

Additionally to ERC-2612 (permit), we may also consider supporting ERC-3009 and/or alternative schemes with adoption.

hellwolf commented 3 months ago

Closing as a spike.