search

Uniswap / v4-core

🦄 🦄 🦄 🦄 Core smart contracts of Uniswap v4
https://blog.uniswap.org/uniswap-v4
2.02k stars 975 forks source link

rm Unnecessary Unlocked Check in Sync #884

Closed wjmelements closed 1 month ago

wjmelements commented 1 month ago

Fixes #883 Sync is already secured by CurrencyReserves. There are many benefits to allowing the transfer-in to happen before unlock. There are no drawbacks. Additionally, this saves gas.

Changes

Removes an unnecessary TLOAD check for sync(). Reviewer @hensha256 @snreynolds

hensha256 commented 1 month ago

Hi! We added this recently on purpose to help with an audit issue :)

wjmelements commented 1 month ago

Hi! We added this recently on purpose to help with an audit issue :)

I noticed. The auditor is probably wrong. Please share their reasoning.

hensha256 commented 1 month ago

The audit reports are all public for you to read https://github.com/Uniswap/v4-core/tree/main/docs/security/audits

wjmelements commented 1 month ago

The audit reports are all public for you to read https://github.com/Uniswap/v4-core/tree/main/docs/security/audits

The only hint is "ToB L01". That doesn't map to any of the items in their audit.

wjmelements commented 1 month ago

In TOB-UNI4-3:

The contract erroneously calls the collectProtocolFees function between sync and settle, and when determining how much needs to be paid to successfully settle the transaction, it manually calculates the currencyDelta.
In this situation, the fee collection process either: 1. becomes a no-op that collects fees and burns them by sending them to the v4 singleton, or 2. the transaction reverts.

This is so avoidable it's funny. Every user can make this mistake. The contract doesn't need to be idiot proof, especially for fee collection. An idiot can always transfer tokens to it and the same problem happens.

If that is the only reason, this change should be merged. Is there another reason?

hensha256 commented 1 month ago

It titled

Collected protocol fees may count against user’s currency deltas

wjmelements commented 1 month ago

If that is the only reason, this change should be merged. Is there another reason?

aparently not:

TOB-UNI4-3: Collected protocol fees may count against user’s currency deltas Resolved in PR 856. A guard was added to the sync function to ensure it can only be called when the contract is in the unlocked state. Additionally, a check was added in the collectProtocolFees function to prevent it from being called when the contract is unlocked, throwing a custom error (ContractUnlocked) if this check fails. Additionally, new tests were added to specifically test for this edge case and to ensure that the updated implementation correctly handles it.

856 is a bad trade-off that should be reverted.

wjmelements commented 1 month ago

I can audit like trail of bits; watch this:

  1. BUG: take() can be called between sync and settle. The took amount will count against the deposit, and the balance is BURNED!!!!
  2. RECOMMENDATION: add guard to take() to make sure the taken currency isn't in the CURRENCY_SLOT

LOW PRIORITY

hensha256 commented 1 month ago

You are consistently rude to people on the uniswap labs protocol team every single time you open a pull request or issue on this repository. You are the only community contributor that we have found to be like this. Please can you reconsider your tone of voice when you speak to us, and be kind and friendly.

Generally we respond well to people using logic and reasoning, and balanced technical arguments. I havent yet seen you make one technical argument about why this PR should be merged? Instead you have resorted to:

  1. calling users idiots
  2. being rude and sarcastic with our team
  3. being rude about our audits

I'll be closing this pull request now. If you want to try to make a new contribution, please do so politely and reason about the tech without being rude :)

wjmelements commented 1 month ago

I think it's a technical term in this context: https://en.wikipedia.org/wiki/Idiot-proof

wjmelements commented 1 month ago

I'll use the term "fool-proof" in the future.

wjmelements commented 1 month ago

Generally we respond well to people using logic and reasoning, and balanced technical arguments. I havent yet seen you make one technical argument about why this PR should be merged?

Perhaps you missed when I did that in the issue linked at the top of the PR: https://github.com/Uniswap/v4-core/issues/883#issuecomment-2380318727

Instead you have resorted to:

  1. calling users idiots

I was using the engineering term "idiot-proof" and back-referencing that term in the same paragraph. We used the term in university and in the workplace. I think perhaps you are just unfamiliar with it.

  1. being rude and sarcastic with our team

I apologize.

  1. being rude about our audits

I have a strong disdain for the pattern of auditors contriving security issues when there is none. The more secure the code the more contrived the issues they raise. I think my mocking of them is justified push-back. However I will refrain from doing so here in the future per your request.

wjmelements commented 1 month ago

I do appreciate the feedback though. I'll try to do better in the future.