Solve ToB-UNI4-3 without restricting sync

[wjmelements]

trafficstars

Solves #883 and ToB-UNI4-3

Reviewers @hensha256 @snreynolds As described in #885 there are advantages to allowing sync to be called outside of the unlock callback. The main reason this ability was removed was to fool-proof collectProtocolFees so that it cannot be called between sync and settle. This PR presents an alternative to #885 that still solves ToB-UNI4-3, not by modifying sync, but by modifying collectProtocolFees. Assuming sync is called more often than collectProtocolFees, this fix for ToB-UNI4-3 is superior to #856 even without considering situations like ERC-4337.

Changes

Restore sync usability outside of the unlock callback. Check in collectProtocolFees whether the token to be transferred out is currently the synced token, handling the native Currency case.

[hensha256]

I quite like this as a solution, nice. Your tests and linting are both failing please can you fix? For tests make sure youve run foundryup as they recently changed their gas snapshotting

[wjmelements]

Forge code coverage:
| File                                         | % Lines            | % Statements       | % Branches       | % Funcs          |
|----------------------------------------------|--------------------|--------------------|------------------|------------------|
| src/ERC6909.sol                              | 82.61% (19/23)     | 78.57% (22/28)     | 100.00% (2/2)    | 85.71% (6/7)     |
| src/ERC6909Claims.sol                        | 100.00% (6/6)      | 100.00% (8/8)      | 100.00% (2/2)    | 100.00% (1/1)    |
| src/Extsload.sol                             | 0.00% (0/28)       | 0.00% (0/30)       | 0.00% (0/2)      | 100.00% (3/3)    |
| src/Exttload.sol                             | 0.00% (0/15)       | 0.00% (0/16)       | 0.00% (0/1)      | 50.00% (1/2)     |
| src/NoDelegateCall.sol                       | 66.67% (2/3)       | 75.00% (3/4)       | 100.00% (1/1)    | 100.00% (3/3)    |
| src/PoolManager.sol                          | 98.98% (97/98)     | 97.78% (132/135)   | 90.48% (19/21)   | 100.00% (20/20)  |
| src/ProtocolFees.sol                         | 84.62% (22/26)     | 86.84% (33/38)     | 71.43% (5/7)     | 100.00% (6/6)    |
| src/libraries/BipsLibrary.sol                | 100.00% (2/2)      | 100.00% (4/4)      | 100.00% (1/1)    | 100.00% (1/1)    |
| src/libraries/BitMath.sol                    | 18.18% (2/11)      | 18.18% (2/11)      | 0.00% (0/4)      | 100.00% (2/2)    |
| src/libraries/CurrencyDelta.sol              | 33.33% (3/9)       | 45.45% (5/11)      | 100.00% (0/0)    | 100.00% (3/3)    |
| src/libraries/CurrencyReserves.sol           | 0.00% (0/5)        | 0.00% (0/5)        | 100.00% (0/0)    | 100.00% (4/4)    |
| src/libraries/CustomRevert.sol               | 0.00% (0/35)       | 0.00% (0/35)       | 100.00% (0/0)    | 100.00% (8/8)    |
| src/libraries/FullMath.sol                   | 68.97% (20/29)     | 72.73% (24/33)     | 33.33% (2/6)     | 100.00% (2/2)    |
| src/libraries/Hooks.sol                      | 94.12% (80/85)     | 95.00% (133/140)   | 92.00% (23/25)   | 100.00% (14/14)  |
| src/libraries/LPFeeLibrary.sol               | 90.00% (9/10)      | 93.75% (15/16)     | 100.00% (1/1)    | 100.00% (7/7)    |
| src/libraries/LiquidityMath.sol              | 0.00% (0/4)        | 0.00% (0/4)        | 0.00% (0/1)      | 100.00% (1/1)    |
| src/libraries/Lock.sol                       | 0.00% (0/3)        | 0.00% (0/3)        | 100.00% (0/0)    | 100.00% (3/3)    |
| src/libraries/NonzeroDeltaCount.sol          | 0.00% (0/7)        | 0.00% (0/7)        | 100.00% (0/0)    | 100.00% (3/3)    |
| src/libraries/ParseBytes.sol                 | 0.00% (0/3)        | 0.00% (0/3)        | 100.00% (0/0)    | 100.00% (3/3)    |
| src/libraries/Pool.sol                       | 94.16% (145/154)   | 93.45% (157/168)   | 98.08% (51/52)   | 100.00% (13/13)  |
| src/libraries/Position.sol                   | 52.63% (10/19)     | 57.14% (12/21)     | 100.00% (3/3)    | 100.00% (3/3)    |
| src/libraries/ProtocolFeeLibrary.sol         | 20.00% (2/10)      | 20.00% (2/10)      | 100.00% (0/0)    | 100.00% (4/4)    |
| src/libraries/SafeCast.sol                   | 100.00% (12/12)    | 100.00% (19/19)    | 100.00% (6/6)    | 100.00% (6/6)    |
| src/libraries/SqrtPriceMath.sol              | 60.42% (29/48)     | 69.84% (44/63)     | 58.33% (7/12)    | 100.00% (9/9)    |
| src/libraries/StateLibrary.sol               | 71.43% (45/63)     | 79.31% (69/87)     | 100.00% (4/4)    | 100.00% (14/14)  |
| src/libraries/SwapMath.sol                   | 80.77% (21/26)     | 81.48% (22/27)     | 100.00% (6/6)    | 100.00% (2/2)    |
| src/libraries/TickBitmap.sol                 | 40.00% (12/30)     | 50.00% (18/36)     | 66.67% (2/3)     | 100.00% (4/4)    |
| src/libraries/TickMath.sol                   | 37.11% (36/97)     | 56.94% (82/144)    | 95.83% (23/24)   | 100.00% (4/4)    |
| src/libraries/TransientStateLibrary.sol      | 70.00% (7/10)      | 76.92% (10/13)     | 100.00% (0/0)    | 100.00% (5/5)    |
| src/libraries/UnsafeMath.sol                 | 0.00% (0/2)        | 0.00% (0/2)        | 100.00% (0/0)    | 100.00% (2/2)    |
| src/types/BalanceDelta.sol                   | 0.00% (0/2)        | 0.00% (0/2)        | 100.00% (0/0)    | 100.00% (2/2)    |
| src/types/BeforeSwapDelta.sol                | 0.00% (0/2)        | 0.00% (0/2)        | 100.00% (0/0)    | 100.00% (2/2)    |
| src/types/Currency.sol                       | 59.09% (13/22)     | 70.00% (21/30)     | 75.00% (6/8)     | 100.00% (6/6)    |
| src/types/PoolId.sol                         | 0.00% (0/1)        | 0.00% (0/1)        | 100.00% (0/0)    | 100.00% (1/1)    |
| src/types/Slot0.sol                          | 0.00% (0/8)        | 0.00% (0/8)        | 100.00% (0/0)    | 100.00% (8/8)    |
| Total                                        | 68.67% (1350/1966) | 71.45% (1817/2543) | 42.28% (274/648) | 82.98% (395/476) |

[snreynolds]

@wjmelements Can you please update this branch?

[wjmelements]

looks good, would you mind also adding a test where the pool manager is unlocked, and a currency is synced and ProtocolFeeCurrencySynced() is thrown?

I have pushed a revert test, e710ad1d, for the locked case that works with forge test but not with forge snapshot --isolate. Do you have any tips? I don't have much experience with Foundry; I normally write and test smart contracts in pure assembly.

[snreynolds]

Can you add back in this modifier and include it on your test. I think thats how we solved this problem in the past https://github.com/Uniswap/v4-core/pull/856/files#r1739134580

[wjmelements]

noIsolate is a pretty clever workaround