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

Address courier front-running and persistent allowances #216

Merged
merged 4 commits into from
Nov 15, 2023

Conversation

haydenshively
Copy link
Member

@haydenshively haydenshively commented Nov 12, 2023

Addresses

The reports above boil down to two points:

  • Since we reuse the allowance mapping to authorize courierId writes, the allowed entity also has control over some quantity of user shares. If users approve a single unit, 1, as intended, this isn't a big deal, but mistakes are possible.
  • The courierId can only be written if the user's balance is precisely 0, so someone could frontrun a user's first deposit to prevent it from being set.

In f27a6b9, I implemented two fixes:

  • To prevent allowance abuse: A separate EIP712 struct hash for permitting courierId writes
  • To make frontrunning innocuous: Allow courierId to be set (from zero to non-zero) even if the user's balance is non-zero

To make that second fix possible, we need the user's principle. This can be done by changing _mint and _burn to track everyone's principle (even if they don't yet have a courier), OR by backfilling the empty principle with _convertToAssets(balance, inventory, totalSupply). Though I implemented the first option, neither is perfect. In the first, couriers receive a cut of all interest, including interest that accrued before they were credited. This feels wrong and users would not expect it. In the second, principle loses its semantic meaning, since the back-filled value may be greater than the sum of all the user's deposited amounts. Even casting these problems aside, the "fix" is a rather large change and increases complexity.

For these reasons, I switched to something simpler:

  • periphery function to make frontrunning irrelevant
  • set allowance to 0 if it was used to set courier

@haydenshively haydenshively changed the title fix couriers Address courier front-running and privilege escalation Nov 12, 2023
@haydenshively haydenshively changed the title Address courier front-running and privilege escalation Address courier front-running and persistent allowances Nov 12, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

1 participant