Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Non-decrementing allowance after recovery to MAX_INT #60

Open
zlace0x opened this issue Dec 12, 2022 · 2 comments
Open

Non-decrementing allowance after recovery to MAX_INT #60

zlace0x opened this issue Dec 12, 2022 · 2 comments
Assignees
Labels
status:invalid This doesn't seem right status:wontfix This will not be worked on

Comments

@zlace0x
Copy link
Contributor

zlace0x commented Dec 12, 2022

After the fix for #30, renewable allowance that recovers to MAX_UINT256 will no longer decrement allowance going forward.

@zlace0x
Copy link
Contributor Author

zlace0x commented Dec 12, 2022

Potential solutions:

  1. Saturating add saturates at MAX_UINT256 - 1 (have to ensure that this applies only when it saturates)
  2. Only skip decrement when initialAmount is MAX_UINT256 (this potentially requires an additional read on state)

@zlace0x zlace0x mentioned this issue Dec 12, 2022
@zlace0x
Copy link
Contributor Author

zlace0x commented Dec 12, 2022

On further investigation, this likely is a non-issue:

Since maxAmount limits the maxRecoverable too, the following scenario cannot happen.

The following test passes given the scenario described here #51

 function testMaxRecovery() public {
      // approves MAX_UINT-1, recovery 1
      vm.prank(user1);
      funnel.approveRenewable(address(spender), type(uint256).max - 1, 1);

      // same block transfer MAX_UINT-1
      vm.prank(address(spender));
      vm.expectEmit(true, false, false, true);
      emit Transfer(user1, user2, type(uint256).max - 1);
      assertTrue(funnel.transferFrom(user1, user2, type(uint256).max - 1));
      assertEq(funnel.allowance(user1, address(spender)), 0);

      vm.warp(2);
      // next block allowance
      assertEq(funnel.allowance(user1, address(spender)), 1);
  }

@zlace0x zlace0x added status:invalid This doesn't seem right status:wontfix This will not be worked on labels Dec 13, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
status:invalid This doesn't seem right status:wontfix This will not be worked on
Projects
None yet
Development

When branches are created from issues, their pull requests are automatically linked.

2 participants