Vanguard
First Flight #56
Beginner FriendlyDeFiFoundry
0 EXP
View all submissions
Previous Next
Submission Details
Impact: medium
Likelihood: high

The `_resetPerAddressTracking` function that is called in `_beforeSwap` never resets address tracking variables, causing penalty fees for users.

Author Revealed upon completion

Root + Impact

Description

  • The _resetPerAddressTracking function is supposed to reset address tracking addressSwappedAmount and addressLastSwapBlock when the currentPhase changes in order to reset previous phase stats.

  • However, these variables are never reset to 0, as _resetPerAddressTracking only resets values for address(0).

function _beforeSwap(address sender, PoolKey calldata key, SwapParams calldata params, bytes calldata)
internal
override
returns (bytes4, BeforeSwapDelta, uint24)
{
.
.
.
if (newPhase != currentPhase) {
@> _resetPerAddressTracking();
currentPhase = newPhase;
lastPhaseUpdateBlock = block.number;
}
.
.
.
​
}
@> function _resetPerAddressTracking() internal {
addressSwappedAmount[address(0)] = 0;
addressLastSwapBlock[address(0)] = 0;
}

Risk

Likelihood:

  • Likelihood is high as there are no specific steps required for this issue to occur.

  • Users swap within the normal flow, performing swaps during various phases.

Impact:

  • Users reach the limits of the next phase sooner than expected, based on stats from the previous phase.

  • Users are penalized for normal operation due to a flaw in the hook logic.

  • Causes denial of service, loss of users as it becomes less interesting for users or potentially more expensive than using other pools.

Proof of Concept

This POC shows that addressSwappedAmount did not reset to 0 after the phase changed.
Add this snippet of code to test/TokenLaunchHookUnit.t.sol

function test_PhaseTransition_Phase1ToPhase2_limitsNotReseted_POC() public {
assertEq(antiBotHook.getCurrentPhase(), 1, "Should start in phase 1");
​
vm.deal(address(this), 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});
swapRouter.swap{value: 0.001 ether}(key, params, testSettings, ZERO_BYTES);
​
// Check that swapRouter has swapped (since sender is the router)
uint256 addressSwappedAmountPhase1 = antiBotHook.addressSwappedAmount(address(swapRouter));
​
vm.roll(block.number + phase1Duration + 1);
​
uint256 currentPhase = antiBotHook.getCurrentPhase();
uint256 addressSwappedAmountPhase2 = antiBotHook.addressSwappedAmount(address(swapRouter));
​
assertEq(currentPhase, 2, "Should be in phase 2");
assertEq(
addressSwappedAmountPhase2,
addressSwappedAmountPhase1,
"SwappedAmount is equal, proves reset on phase change did not happen"
);
}

Recommended Mitigation

  1. Add sender address input parameter to _resetPerAddressTracking, and replace address(0) with this new input parameter.

  2. Replace _resetPerAddressTracking(); with _resetPerAddressTracking(sender); in the _beforeSwap function.

function _beforeSwap(address sender, PoolKey calldata key, SwapParams calldata params, bytes calldata)
internal
override
returns (bytes4, BeforeSwapDelta, uint24)
{
.
.
.
if (newPhase != currentPhase) {
- _resetPerAddressTracking();
+ _resetPerAddressTracking(sender);
currentPhase = newPhase;
lastPhaseUpdateBlock = block.number;
}
.
.
.
}
​
- function _resetPerAddressTracking() internal {
- addressSwappedAmount[address(0)] = 0;
- addressLastSwapBlock[address(0)] = 0;
+ function _resetPerAddressTracking(address sender) internal {
+ addressSwappedAmount[sender] = 0;
+ addressLastSwapBlock[sender] = 0;
}

Support

FAQs

Can't find an answer? Chat with us on Discord, Twitter or Linkedin.

Give us feedback!