Root + Impact
Description
-
The _afterSwap function calculation of current phase and getCurrentPhase should match
-
Two functions have similar calculations with small variation, the _afterSwap counts for phase equality between blocksSinceLaunch and phase durations, however getCurrentPhase does not, comparing only greater or lower values.
The code snippet from _beforeSwap function.
function _beforeSwap(address sender, PoolKey calldata key, SwapParams calldata params, bytes calldata)
internal
override
returns (bytes4, BeforeSwapDelta, uint24)
{
@> if (blocksSinceLaunch <= phase1Duration) {
newPhase = 1;
@> } else if (blocksSinceLaunch <= phase1Duration + phase2Duration) {
newPhase = 2;
} else {
newPhase = 3;
}
.
.
.
}
The code snippet from getCurrentPhase function.
function getCurrentPhase() public view returns (uint256) {
if (launchStartBlock == 0) return 0;
uint256 blocksSinceLaunch = block.number - launchStartBlock;
@> if (blocksSinceLaunch < phase1Duration) {
return 1;
@> } else if (blocksSinceLaunch < phase1Duration + phase2Duration) {
return 2;
} else {
return 3;
}
}
Risk
Likelihood:
Impact:
-
Impact is low, mostly and does not cause loss of funds but may affect user decision to commit swap.
-
In case of deterministic approach, getter function should reflect the actual logic if provided.
Proof of Concept
function test_swapPhase_does_not_match_getCurrentPhase() public {
vm.roll(block.number + phase1Duration + phase2Duration);
uint256 initialPhase = antiBotHook.currentPhase();
uint256 currentPhase = antiBotHook.getCurrentPhase();
uint256 user1SwapAmount = 1 ether;
vm.deal(user1, user1SwapAmount);
vm.startPrank(user1);
SwapParams memory paramsUser1 = SwapParams({
zeroForOne: true, amountSpecified: -int256(user1SwapAmount), sqrtPriceLimitX96: TickMath.MIN_SQRT_PRICE + 1
});
PoolSwapTest.TestSettings memory testSettingsUser1 =
PoolSwapTest.TestSettings({takeClaims: false, settleUsingBurn: false});
swapRouter.swap{value: user1SwapAmount}(key, paramsUser1, testSettingsUser1, ZERO_BYTES);
vm.stopPrank();
uint256 swapPhase = antiBotHook.currentPhase();
console.log("swap phase: ", swapPhase);
console.log("intial phase state: ", initialPhase);
console.log("current phase state: ", currentPhase);
assertNotEq(antiBotHook.currentPhase(), antiBotHook.getCurrentPhase(), "Should be equal");
}
result:
swap phase: 2
intial phase state: 1
current phase state: 3
Recommended Mitigation
Add equality to getCurrentPhase function comparison of blocks passed and phase duration.
function getCurrentPhase() public view returns (uint256) {
if (launchStartBlock == 0) return 0;
uint256 blocksSinceLaunch = block.number - launchStartBlock;
- if (blocksSinceLaunch < phase1Duration) {
+ if (blocksSinceLaunch <= phase1Duration) {
return 1;
- } else if (blocksSinceLaunch < phase1Duration + phase2Duration) {
+ } else if (blocksSinceLaunch <= phase1Duration + phase2Duration) {
return 2;
} else {
return 3;
}
}
