Root + Impact
-
In _beforeSwap, addressSwappedAmount and addressLastSwapBlock are keyed by the sender parameter, which typically resolves to the Uniswap router rather than the actual end user.
-
As a result, swap limits and cooldowns are enforced at the router level instead of per user, causing users to be penalized based on the cumulative activity of others, or even without having traded themselves at all!
Description
-
_beforeSwap is intended to track per-user swap amounts and last swap blocks across phases in order to enforce limits and cooldowns.
-
However, both addressSwappedAmount and addressLastSwapBlock are updated using the sender parameter, which usually represents a router or intermediary contract rather than the user.
-
This causes most swaps to be attributed to a single address (or a small set of intermediaries), resulting in shared limits and incorrect cooldown enforcement across multiple users.
function _beforeSwap(
address sender,
PoolKey calldata key,
SwapParams calldata params,
bytes calldata)
internal
override
returns (bytes4, BeforeSwapDelta, uint24){
@> if (addressLastSwapBlock[sender] > 0) {
uint256 blocksSinceLastSwap = block.number - addressLastSwapBlock[sender];
if (blocksSinceLastSwap < phaseCooldown) {
applyPenalty = true;
}
}
@> if (!applyPenalty && addressSwappedAmount[sender] + swapAmount > maxSwapAmount) {
applyPenalty = true;
}
@> addressSwappedAmount[sender] += swapAmount;
@> addressLastSwapBlock[sender] = block.number;
}
Risk
Likelihood: High
Impact: High
Proof of Concept
function test_MappingsOnlyUseRouterAddr() public {
vm.deal(user1, 1 ether);
vm.deal(user2, 1 ether);
SwapParams memory params = SwapParams({
zeroForOne: true,
amountSpecified: -int256(0.001 ether),
sqrtPriceLimitX96: TickMath.MIN_SQRT_PRICE + 1
});
PoolSwapTest.TestSettings memory testSettings =
PoolSwapTest.TestSettings({takeClaims: false, settleUsingBurn: false});
uint256 initialUser1TokenBalance = token.balanceOf(user1);
uint256 initialUser2TokenBalance = token.balanceOf(user2);
vm.startPrank(user1);
token.approve(address(swapRouter), type(uint256).max);
swapRouter.swap{value: 0.001 ether}(key, params, testSettings, ZERO_BYTES);
vm.stopPrank();
vm.roll(block.number + 1);
vm.startPrank(user2);
token.approve(address(swapRouter), type(uint256).max);
swapRouter.swap{value: 0.001 ether}(key, params, testSettings, ZERO_BYTES);
vm.stopPrank();
uint256 user1Delta = token.balanceOf(user1) - initialUser1TokenBalance;
uint256 user2Delta = token.balanceOf(user2) - initialUser2TokenBalance;
assertGt(user1Delta, user2Delta, "User1 should receive more tokens than User2 due to cooldown penalty");
uint256 penaltyBps = phase1PenaltyBps;
uint256 expectedPenalty = (0.001 ether * penaltyBps) / 10_000;
assertGt(user1Delta, user2Delta + expectedPenalty, "User2 should receive fewer tokens due to cooldown penalty");
}
Recommended Mitigation
- function _beforeSwap(address sender, PoolKey calldata key, SwapParams calldata params, bytes calldata)
+ function _beforeSwap(address sender, PoolKey calldata key, SwapParams calldata params, bytes calldata data)
internal
override
returns (bytes4, BeforeSwapDelta, uint24)
{
// ... existing code ...
bool applyPenalty = false;
+ address trader = sender; // default to sender
+ if (data.length == 32) {
+ trader = abi.decode(data, (address));
+ }
- if (addressLastSwapBlock[sender] > 0) {
+ if (addressLastSwapBlock[trader] > 0) {
- uint256 blocksSinceLastSwap = block.number - addressLastSwapBlock[sender];
+ uint256 blocksSinceLastSwap = block.number - addressLastSwapBlock[trader];
if (blocksSinceLastSwap < phaseCooldown) {
applyPenalty = true;
}
}
- if (!applyPenalty && addressSwappedAmount[sender] + swapAmount > maxSwapAmount) {
+ if (!applyPenalty && addressSwappedAmount[trader] + swapAmount > maxSwapAmount) {
applyPenalty = true;
}
- addressSwappedAmount[sender] += swapAmount;
- addressLastSwapBlock[sender] = block.number;
+ addressSwappedAmount[trader] += swapAmount;
+ addressLastSwapBlock[trader] = block.number;
uint24 feeOverride = 0;
if (applyPenalty) {
feeOverride = uint24((phasePenaltyBps * 100));
}
return (
BaseHook.beforeSwap.selector,
BeforeSwapDeltaLibrary.ZERO_DELTA,
feeOverride | LPFeeLibrary.OVERRIDE_FEE_FLAG
);
}
