Giter VIP home page Giter VIP logo

permit2's People

Contributors

alexfertel avatar apoorvlathey avatar ewilz avatar fubuloubu avatar hensha256 avatar marktoda avatar merlinegalite avatar noahzinsmeister avatar paulrberg avatar shungy avatar snreynolds avatar transmissions11 avatar

Stargazers

 avatar  avatar  avatar  avatar  avatar  avatar  avatar  avatar  avatar  avatar  avatar  avatar  avatar  avatar  avatar  avatar  avatar  avatar  avatar  avatar  avatar  avatar  avatar  avatar  avatar  avatar  avatar  avatar  avatar  avatar  avatar  avatar  avatar  avatar  avatar  avatar  avatar  avatar  avatar  avatar  avatar  avatar  avatar  avatar  avatar  avatar  avatar  avatar  avatar  avatar  avatar  avatar  avatar  avatar  avatar  avatar  avatar  avatar  avatar  avatar  avatar  avatar  avatar  avatar  avatar  avatar  avatar  avatar  avatar  avatar  avatar  avatar  avatar  avatar  avatar  avatar  avatar  avatar  avatar  avatar  avatar  avatar  avatar  avatar  avatar  avatar  avatar  avatar  avatar  avatar  avatar  avatar  avatar  avatar  avatar  avatar  avatar  avatar  avatar  avatar

Watchers

 avatar  avatar  avatar  avatar  avatar  avatar  avatar  avatar  avatar  avatar  avatar  avatar  avatar  avatar  avatar  avatar  avatar  avatar

permit2's Issues

Small test code bug

AllowanceTransferTest.t.sol

testBatchTransferFromMultiToken
startBalanceTo1 is token0.balanceOf(address0); - should be: token1.balanceOf(address0);
assertEq(token0.balanceOf(address0), startBalanceTo1 + 1 ** 18); should be assertEq(token1.balanceOf(address0), startBalanceTo1 + 1 ** 18);

tests still pass when corrected

allowance is not being updated when 2**160-1 is approved

i was wondering in _transfer whether the bypass of updating the allowed.amount when approved amount is 2**160-1 is intended to save on gas, as i have tried to test in 2 cases, for any value under the max, it did deduct from the allowance accordingly. and also I was looking into the test cases and there is a lack of checking for allowance when permit2.transferFrom takes place.

ERC4626 Support

[EDIT]: only withdraw and redeem methods in ERC-4626 use the underlying ERC-20 approval, so I edited the below to reflect that. This also simplifies the permit2 integration.

Similar to Uniswap/universal-router#176, I believe ERC-4626 integration directly into permit2 is highly beneficial. The main reasons are:

  1. natural integration - ERC-4626 leverages the native ERC-20 approval system (and by extension permit and permit2) for mutable share exit methods (withdraw and redeem), so naturally benefits greatly
  2. gas savings - Integrations like universalRouter can save gas by directly plugging into ERC-4626 mutable methods rather than transferring shares around (which can be costly if the vault includes custom transfer logic, such as in Compound). New vaults could even default approve permit2 for further user gas savings.
  3. contract size reduction - New ERC-4626 contracts need not include EIP-2612 logic.

Given that permit2 is intended to be deployed only once to save user approvals on gas, I'd lobby for ERC-4626 inclusion in the canonical deployment, whereas on universal router it can be added in a later deployment.

The simplest integration would be to only replicate the single-use transferFrom logic in AllowanceTransfer.sol for both of the withdraw and redeem functions. Batch methods and SignatureTransfer could also be added for each, although I'm not sure if the extra complexity is warranted

Can I get a sense for the intended freeze date for the permit2 contract? If the timelines are possible, I would be able to have a tested PR ready before the end of next week.

optional optis: grouping multiple arrays into struct arrays

struct PermitBatch {
// ERC20 token addresses
address[] tokens;
// address permissioned on the allowed tokens
address spender;
// the maximum amounts allowed to spend per token
uint160[] amounts;
// timestamp at which a spender's token allowances become invalid, assigned per token
uint64[] expirations;

address[] tokens;
// address permissioned to spend tokens
address spender;
// the maximum amounts that can be spent per token
uint256[] signedAmounts;

Possible improvement Permit2Lib.sol:permit2

I think it could be better instead of check the DOMAIN_SEPARATOR which some tokens that implement permit do not implement like UNI.
Check instead the PERMIT_TYPEHASH in a very similar behaviour.
In this way you can support native permit to the tokens that do not support DOMAIN_SEPARATOR like UNI ( so overall will be more optimal) and also support other DAI like tokens that support permit, that you see in this list

EDIT: Also the DOMAIN_SEPARATOR will change in every network, so you should update the DAI_DOMAIN_SEPARATOR constant in case of a deployment in different networks, but the PERMIT_TYPEHASH won't change ^^

Can set expired allowances

In the library Allowance, neither updateAll nor updateAmountAndExpiration check that the expiration passed is not in the past.

That means it's possible to set an approval expiration timestamp that ends up being lower than block.timestamp (and greater than 0, which is the special case), and have the call succeed.

Seems like a minor oversight - I don't think it has worrying consequences. As long as user sets a reasonable expiration for the allowance, things should be fine. Worst case scenario the expired allowance is stored, the user notices the problem, and submits another tx to update the allowance.

Here's a simple test case to reproduce:

function testApproveExpired() public {
    vm.warp(block.timestamp + 1000);
    uint48 expiredTimestamp = uint48(block.timestamp - 1);
    vm.prank(from);
    vm.expectEmit(true, true, true, true);
    emit Approval(from, address(token0), address(this), defaultAmount, expiredTimestamp);
    permit2.approve(address(token0), address(this), defaultAmount, expiredTimestamp);

    (uint160 amount, uint48 expiration, uint48 nonce) = permit2.allowance(from, address(token0), address(this));
    assertEq(amount, defaultAmount);
    assertEq(expiration, expiredTimestamp);
    assertEq(nonce, 0);
}

Staticcall to fallback function burns all gas

On line 90 of Permit2Lib there is an attempt to do a staticcall to get the DOMAIN_SEPARATOR value. If the token does not have a DOMAIN_SEPARATOR, the call fails and the function falls back to using permit2. However, if the token has a fallback function (such as WETH), all gas sent in the static call gets burned.

This can have negative effects if a user sets a high gasLimit in their transaction. For example, while testing this function my test environment had a default gas limit equal to the block limit, so the transaction used over 29 million gas.

You can see a similar issue come up in this StackExchange post: https://ethereum.stackexchange.com/questions/96547/high-gas-consumption-when-using-staticcall-on-a-non-view-pure-function

Is allowance required?

I am wondering if the transfer by signature logic introduced here in combination with EIP 1271 removes the need for the approve + transferFrom flow and the allowance storage in the ERC20 standard if we were to start from scratch? Or is there some special cases when allowance still has extended functionality?

In a slightly tangential yet related note, is there any thought about introducing a transfer by signature scheme for custom defined data such as a UniswapV3Pool position. The benefit of this would be that custody of the position could be separated from the helper functions that allow you to enter and exit a position. I think you could then still go back and wrap this in the ERC721 standard after the fact too.

multiple arrays that must be same length is better represented as array of structs

// ERC20 token addresses
address[] tokens;
// address permissioned on the allowed tokens
address spender;
// the maximum amounts allowed to spend per token
uint160[] amounts;
// timestamp at which a spender's token allowances become invalid, assigned per token
uint64[] expirations;

for readability, this might better be represented as array of structs, something like:

struct Permit {
  address token;
  uint32 nonce;
  uint160 amount;
  uint64 expiry;
}

struct PermitSingle {
  Permit permit;
  address spender;
}

struct PermitBatch {
  Permit[] permits;
  address spender;
}

Recommend Projects

  • React photo React

    A declarative, efficient, and flexible JavaScript library for building user interfaces.

  • Vue.js photo Vue.js

    ๐Ÿ–– Vue.js is a progressive, incrementally-adoptable JavaScript framework for building UI on the web.

  • Typescript photo Typescript

    TypeScript is a superset of JavaScript that compiles to clean JavaScript output.

  • TensorFlow photo TensorFlow

    An Open Source Machine Learning Framework for Everyone

  • Django photo Django

    The Web framework for perfectionists with deadlines.

  • D3 photo D3

    Bring data to life with SVG, Canvas and HTML. ๐Ÿ“Š๐Ÿ“ˆ๐ŸŽ‰

Recommend Topics

  • javascript

    JavaScript (JS) is a lightweight interpreted programming language with first-class functions.

  • web

    Some thing interesting about web. New door for the world.

  • server

    A server is a program made to process requests and deliver data to clients.

  • Machine learning

    Machine learning is a way of modeling and interpreting data that allows a piece of software to respond intelligently.

  • Game

    Some thing interesting about game, make everyone happy.

Recommend Org

  • Facebook photo Facebook

    We are working to build community through open source technology. NB: members must have two-factor auth.

  • Microsoft photo Microsoft

    Open source projects and samples from Microsoft.

  • Google photo Google

    Google โค๏ธ Open Source for everyone.

  • D3 photo D3

    Data-Driven Documents codes.