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

Assumption of limits being tracked for each user is broken

Author Revealed upon completion

Root +Impact

Description

  • Even reading the documentation it remains unclear if the intention is to track limits and addressSwappedAmount for each user separately

  • In the following test found in the project's test suite TokenLaunchHookUnit.t.sol , the function name test_Phase1_MultipleUsersRespectiveLimits seems to imply that tracking each user separately is the intention, however the test checks for addressSwappedAmount(swapRouter) which will accumulate amounts for all of the users:

function test_Phase1_MultipleUsersRespectiveLimits() public {
vm.deal(user1, 10 ether);
vm.deal(user2, 10 ether);
​
uint256 swapAmount = 0.001 ether;
​
vm.startPrank(user1);
SwapParams memory params = SwapParams({
zeroForOne: true,
amountSpecified: -int256(swapAmount),
sqrtPriceLimitX96: TickMath.MIN_SQRT_PRICE + 1
});
​
PoolSwapTest.TestSettings memory testSettings =
PoolSwapTest.TestSettings({takeClaims: false, settleUsingBurn: false});
​
swapRouter.swap{value: swapAmount}(key, params, testSettings, ZERO_BYTES);
vm.stopPrank();
​
vm.startPrank(user2);
swapRouter.swap{value: swapAmount}(key, params, testSettings, ZERO_BYTES);
vm.stopPrank();
​
assertGt(
antiBotHook.addressSwappedAmount(address(swapRouter)), swapAmount, "Router should have accumulated swaps"
);
}

Risk

Likelihood

  • High: The TokenLaunchHook contract will always track addressSwappedAmount cumulative amounts for swapRouter and not separately for each user, same for the variable addressLastSwapBlock.

Impact

  • High: If the intention is to match each user with its respective addressSwappedAmount or addressLastSwapBlock the hook won't work as intended.

  • Only the users that sell first won't be penalized, effectively favoring bots.

Proof of concept

Add to the following lines to the test mentioned above, it will demonstrate that the addressSwappedAmount for the 2 users respectively remains "0":

assertEq(
antiBotHook.addressSwappedAmount(address(user1)),
0,
"User 1 addressSwappedAmount should stay 0"
);
assertEq(
antiBotHook.addressSwappedAmount(address(user2)),
0,
"User 2 addressSwappedAmount should stay 0"
);

Recommended mitigation

If the intention is to track addressSwappedAmount and addressLastSwapBlock separately for each user, in the function _beforeSwap the address of the user that calls the swap should be indentified by decoding the calldata argument.

function _beforeSwap(
address sender,
PoolKey calldata key,
SwapParams calldata params,
- bytes calldata /*hookData*/
+ bytes calldata hookData
) external override returns (bytes4, BeforeSwapDelta, uint24) {
+ // Extract the actual swapper from hookData
+ address trueUser = hookData.length >= 32 ? abi.decode(hookData, (address)) : sender;
​
- addressSwappedAmount[sender] += amount;
+ addressSwappedAmount[trueUser] += amount;
...
...
}

To prevent a malicious swapRouter to fabricate transactions with misleading addresses to avoid limits, a whitelist of trustes "swap routers" should be maintained:

mapping(address => bool) public isTrustedRouter;
​
function setRouterWhitelists(address router, bool status) external onlyOwner {
isTrustedRouter[router] = status;
}
​
// Inside beforeSwap...
address trueUser;
if (isTrustedRouter[sender]) {
trueUser = abi.decode(hookData, (address));
} else {
// If not a trusted router, revert
trueUser = sender;
}

The tests should be updated accordingly.

Support

FAQs

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

Give us feedback!