Can set expired allowances

[tinchoabbate]

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

[marktoda]

Interesting point - thanks for raising!

My initial take is that the behavior is exactly the same whether we revert on already-expired allowance or succeed and store it anyways. Reverting saves a little gas for the user who made the mistake, and is a little more clear that the approval is not going to be usable. But I'm not sure it's even worth the added (admittedly tiny) gas to every other permit/approval to check+revert in this rare case. Esp given its a check that can/will be easily caught in interfaces/sdk integrations

[marktoda]

@snreynolds want to double check my thoughts here?