uniswap / permit2 Goto Github PK

let's leave this until the very end incase the interface changes at all

Include nonce as indexed in event

[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.

check that the lengths of tokens, amounts, and expirations are equal

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

permit2 and transferFrom2 take uint256 amount as an argument but permit accepts uint160 amount, so breaks for inputs greater than 2**160

we should include overflow checks

might be kind of odd and annoying from an integrator's pov to reveal this internal storage detail to the outside world. could encapsulate and just do a safe cast if outside the limit

some tokens don't revert but return false on transferFrom calls in which case allowances may still be updated

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

otherwise it'll just be invalid signature error

compare gas overhead between permit single and permitBatch of len 1

Currently this is treated as success but should not be

the readme could use some love

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;
}

remove spender param and pass in msg.sender in the sig reconstruction

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);
}

We don't care about injections so should just be combined

every function seems to have a batched version, maybe approve should as well? thoughts?

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.

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 ^^

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.

should benchmark typehashes against eth_signTypedData_v4

Should send funds to spender if 0 addr is passed in as recipient

given the results we've seen in #62, might be a good idea to update all the similar batch functions to have similar interfaces?

Bench if a struct for token-spender pairs is cheaper

this isn't stored anywhere so larger types cheaper