protocol-monorepo icon indicating copy to clipboard operation
protocol-monorepo copied to clipboard

[ETHEREUM-CONTRACTS] System of Approvals Spike

Open 0xdavinchee opened this issue 1 year ago • 13 comments

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:

  • Approve maybe could work for distribute() as well
  • ACL could work for CFA and GDA
  • Maybe we get rid of the operator?
  • Maybe we explore permit/permit2 whatever new stuff exists

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

  • security
  • account abstraction
  • extensibility

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

0xdavinchee avatar Jun 28 '23 10:06 0xdavinchee

Maybe we get rid of the operator?

yes. any time.

hellwolf avatar Jun 28 '23 11:06 hellwolf

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 avatar Jun 28 '23 11:06 hellwolf

Maybe we explore permit/permit2 whatever new stuff exists

Continue to watch on the side line:

  • the polygon usage can be found here: https://polygonscan.com/address/0x000000000022d473030f116ddee9f6b43ac78ba3

image.png

  • mainnet usage: https://etherscan.io/address/0x000000000022d473030f116ddee9f6b43ac78ba3

image.png

hellwolf avatar Jun 28 '23 11:06 hellwolf

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 avatar Jun 28 '23 11:06 hellwolf

@vmichalik

hellwolf avatar Jun 28 '23 11:06 hellwolf

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 avatar Jul 26 '23 18:07 d10r

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

Ethereum

image

image

Polygon

image

image

d10r avatar Jul 26 '23 18:07 d10r

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:

  • https://eips.ethereum.org/EIPS/eip-2612
  • https://mirror.xyz/0xf9b0D66d701151366Dd32A6F0467ffF64f847156/51zh5eo-EZaopCJ8Xic7tqAGHGChEzxYWy5tWjA9zQI

kasparkallas avatar Aug 28 '23 08:08 kasparkallas

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

philipandersson avatar Aug 28 '23 09:08 philipandersson

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:

  • SignatureTransfer
    • permitTransferFrom, with 2 variants of either singleton SignatureTransferDetails or SignatureTransferDetails[],
    • permitWitnessTransferFrom, similarly with 2 variants too.
  • IAllowanceTransfer
    • ... usual erc20 approval system

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

  • Permit2-enhanced Super Token Upgrade:
    • Auto-wrap, when underlying supports Permit2 (?).

hellwolf avatar Aug 28 '23 09:08 hellwolf

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

  • permit2 is likely a piece of a larger solution
  • solving wrapping/autowrapping would solve a lot of more fundamental ux issues w/ superfluid

What do I want as the user?

  • nobody wants to repeat approval multiple times
  • probably not unlimited approval
  • a high enough approval instead

possible solutions:

  • Adding Permit2 into SuperToken.upgrade?
  • Adding upgradeFrom/upgradeFor function into SuperToken contract?
    • Currently: transferFrom to contract, upgrade OR upgradeTo
    • What allowance do we use?
      • same as transfer: danger of people
      • different
      • universal approval where you give approval to protocol, anyone can upgrade on behalf (completely permissionless)
  • possibly rewriting autowrap which may also make it clear that it can be added to the protocol
    • restrict upgrade based on balance/flowrate/deposit: can we set unlimited approval to an non-upgradeable contract to upgrade on your behalf, we don't want to expose the underlying erc20 token balance
      • limit the amount to be upgraded, e.g. in autowrap: 7 days flow rate
        • configuration (in autowrap): upper limit or lower limit based on time
      • where do we put the time-based allowance for upgrading
    • flow:
      • time-based allowance stored in underlying w/ permit2 (1 txn)
      • how does the nonupgradeable autowrap2 know which is the supertoken contract
      • store time-based allowance in autowrap (2 txns)
    • important to prevent: governance messing with time-based allowance
    • in theory we could encode time-based limit in the underlying?
    • permission would be hardcoded to a specific address (autowrap contract)
    • time-based allowance stored in non-upgradeable contract OR underlying
      • a sweet spot
    • users can opt into the non-upgradeable autowrap, admin function for setting up pointers to supertokens
    • unifying allowances

simple poc

  • super simple autowrap which integrates permit2

end game ideas

  • one non-upgradeable contract for all supertokens w/ permit2
  • use gas tokens instead of supertoken
  • a gastank autowrap, users can send gas tokens to the contract
    • hooks to see how

resources

  • https://blog.uniswap.org/permit2-and-universal-router
  • https://blog.uniswap.org/permit2-integration-guide

0xdavinchee avatar Aug 28 '23 11:08 0xdavinchee

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:

  • 2 sources of truth for ERC-20 allowance (Auto-Wrap and Protocol) which could be replaced with a single contract that handles both Auto-Wrap and direct msg.sender wrapping
  • Unupgradeable for better security
  • Lessen on-chain transaction count by using Permit2
  • Unnecessarily complicated Auto-Wrap contract that's not a batchable SuperApp
  • The current Auto-Wrap contract requires explicit transaction to set-up a "wrapping schedule" (i.e. only allowance should be enough)
  • The Auto-Wrap could possibly be improved with a payable based Gas Tank mechanism to incentivize permisonless invocation without explicitly setting up an automation

kasparkallas avatar Aug 28 '23 11:08 kasparkallas

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.

d10r avatar Aug 28 '23 16:08 d10r

Closing as a spike.

hellwolf avatar Jul 31 '24 08:07 hellwolf