dimulski - LV token holders receive proportional fees, when they shouldn't

[sherlock-admin3]

dimulski

High

LV token holders receive proportional fees, when they shouldn't

Summary

The Corc protocol claims that the LV token holders should accrue fees that the protocol generates, from the docs: In our design we envision several mechanisms through which the Liquidity Vault will generate revenues. Some of these are from protocol fees that will flow to the Liquidity Vault and accrue to Liquidity Vault tokenholders. However, the fee accrual is incorrect, a user can deposit just before fees are about to be distributed to LV holders, and still receive the same amount of fees as a user who deposited tokens in the beginning. There is one LV token per RA:PA pair, however the CT and DS tokens expire and there may be several CT and DS tokens issued by the protocol. Users can mint an LV token via the Vault::depositLv() function, by depositing the corresponding RA asset. The first issue is that if User A mints a LV token, and request to redeem it, then some fees from swapping in the AMM pair for the CT and RA token are generated, or the protocol collects fees from users utilizing its functionality, then a User B mints a LV token, and request to redeem it, both users will accrues the same amount of fees generated by the protocol, when this shouldn't be the case. As the first users has had his request for redeem for a much longer period, risking the price of the RA token dropping significantly, while User B may just call the Vault::depositLv() function the last block before the CT and DS tokens expire, and request a redeem immediately. Then in the next block when the CT and DS tokens have already expired he can claim the same amount of fees as User A, this is demonstrated in the first POC. To better illustrate the second problem consider the following example

• User A and User B minted LV tokens while the first issued CT and DS tokens were still not expired
• The UniV2 pair generated some fees
• The protocol collects fees, from users utilizing its functionality
• The issued CT and DS tokens expired
• The protocol issued new CT and DS tokens
• User C mints some LV tokens
• The UniV2 pair generates some more fees,

The fees will be split equally between all the users (of course taking into account the amount of LV tokens they hold).

Root Cause

The protocol doesn't implement any mechanism to track when users minted LV tokens, or when they requested a redemption of their LV tokens, or what rewards were accrued to the current LV holders.

Internal pre-conditions

No response

External pre-conditions

No response

Attack Path

No response

Impact

Fees are distrusted incorrectly, users who have held LV tokens since the beginning will receive an equal amount of fees with users who deposit much later, even if no fees are generated by the protocol since the last user deposited. The last users to deposit are effectively stealing fees from the users who deposited earlier.

PoC

Gist
After following the steps in the above mentioned gist add the following tests to the AuditorTests.t.sol file:

POC 1

    function test_IncorrectFeeAccrural() public {
        vm.startPrank(alice);
        WETH.mint(alice, 1e18);
        WETH.approve(address(moduleCore), type(uint256).max);
        moduleCore.depositLv(id, 1e18);
        Asset(lvAddress).approve(address(moduleCore), type(uint256).max);
        moduleCore.requestRedemption(id, 1e18);
        vm.stopPrank();

        /// @notice add 1e18 WETH to the amm pair, imagine this is generated from fees
        IUniswapV2Pair univ2Pair = flashSwapRouter.getUniV2pair(id, 1);
        WETH.mint(address(univ2Pair), 1e18);

        vm.startPrank(bob);
        WETH.mint(bob, 1e18);
        WETH.approve(address(moduleCore), type(uint256).max);
        moduleCore.depositLv(id, 1e18);
        Asset(lvAddress).approve(address(moduleCore), type(uint256).max);
        moduleCore.requestRedemption(id, 1e18);
        vm.stopPrank();

        /// @notice skip 1100 seconds so the CT and DS tokens expire
        skip(1100);

        vm.startPrank(alice);
        console2.log("WETH balance of alice before she redeems: ", WETH.balanceOf(alice));
        moduleCore.redeemExpiredLv(id, alice, 1e18);
        console2.log("WETH balance of alice after she has redeemed: ", WETH.balanceOf(alice));
        vm.stopPrank();

        vm.startPrank(bob);
        console2.log("WETH balance of bob before he redeems: ", WETH.balanceOf(bob));
        moduleCore.redeemExpiredLv(id, bob, 1e18);
        console2.log("WETH balance of bob after he has redeemed: ", WETH.balanceOf(bob));
        assertEq(WETH.balanceOf(alice), WETH.balanceOf(bob));
        vm.stopPrank();
    }

Logs:
  WETH balance of alice before she redeems:  0
  WETH balance of alice after she has redeemed:  1499999999999998497
  WETH balance of bob before he redeems:  0
  WETH balance of bob after he has redeemed:  1499999999999998497

To run the test use: forge test -vvv --mt test_IncorrectFeeAccrural

POC 2

    function test_IncorrectFeeAccruralBetweenDSIssuings() public {
        vm.startPrank(alice);
        WETH.mint(alice, 1e18);
        WETH.approve(address(moduleCore), type(uint256).max);
        moduleCore.depositLv(id, 1e18);
        Asset(lvAddress).approve(address(moduleCore), type(uint256).max);
        moduleCore.requestRedemption(id, 1e18);
        vm.stopPrank();

        /// @notice add 1e18 WETH to the amm pair, imagine this is generated from fees
        IUniswapV2Pair univ2Pair = flashSwapRouter.getUniV2pair(id, 1);
        WETH.mint(address(univ2Pair), 1e18);

        vm.startPrank(bob);
        WETH.mint(bob, 1e18);
        WETH.approve(address(moduleCore), type(uint256).max);
        moduleCore.depositLv(id, 1e18);
        Asset(lvAddress).approve(address(moduleCore), type(uint256).max);
        moduleCore.requestRedemption(id, 1e18);
        vm.stopPrank();

        vm.startPrank(owner);
        /// @notice the first issuance of DS and Ct tokens expires
        skip(1100);
        corkConfig.issueNewDs(id, block.timestamp + expiry, 1e18, 5e18);
        vm.stopPrank();

        vm.startPrank(tom);
        WETH.mint(tom, 1e18);
        WETH.approve(address(moduleCore), type(uint256).max);
        moduleCore.depositLv(id, 1e18);
        Asset(lvAddress).approve(address(moduleCore), type(uint256).max);
        moduleCore.requestRedemption(id, 1e18);
        vm.stopPrank();

        /// @notice add 1e18 WETH to the amm pair, imagine this is generated from fees
        WETH.mint(address(univ2Pair), 0.3e18);
        skip(1100);

        vm.startPrank(alice);
        console2.log("WETH balance of alice before she redeems: ", WETH.balanceOf(alice));
        moduleCore.redeemExpiredLv(id, alice, 1e18);
        console2.log("WETH balance of alice after she has redeemed: ", WETH.balanceOf(alice));
        vm.stopPrank();
        
        vm.startPrank(bob);
        console2.log("WETH balance of bob before he redeems: ", WETH.balanceOf(bob));
        moduleCore.redeemExpiredLv(id, bob, 1e18);
        console2.log("WETH balance of bob after he has redeemed: ", WETH.balanceOf(bob));
        assertEq(WETH.balanceOf(alice), WETH.balanceOf(bob));
        vm.stopPrank();

        vm.startPrank(tom);
        console2.log("WETH balance of tom before he redeems: ", WETH.balanceOf(tom));
        moduleCore.redeemExpiredLv(id, tom, 1e18);
        console2.log("WETH balance of tom after he has redeemed: ", WETH.balanceOf(tom));
        assertEq(WETH.balanceOf(alice), WETH.balanceOf(tom));
        vm.stopPrank();
    }

Logs:
  WETH balance of alice before she redeems:  0
  WETH balance of alice after she has redeemed:  1333333333333331663
  WETH balance of bob before he redeems:  0
  WETH balance of bob after he has redeemed:  1333333333333331663
  WETH balance of tom before he redeems:  0
  WETH balance of tom after he has redeemed:  1333333333333331663

To run the test use: forge test -vvv --mt test_IncorrectFeeAccruralBetweenDSIssuings

Mitigation

No response

[ziankork]

This is fixed, by implementing an exchange rate for the LV (will add the link to it later)

[cvetanovv]

No impact. The protocol works as intended.

[AtanasDimulski]

Escalate,
I don't agree that there is no impact. As I have mentioned in the impact section of the report, the last users who mint LV tokens via deposits will be stealing funds from the first users. This is not how rewarding contracts work. One user can mint LV tokens in the first round, and if 10 rounds pass and then someone else mints LV tokens, they will receive the same rewards. This is clearly stealing rewards from users. The protocol has confirmed and fixed it, so any claim that this is a design decision is incorrect.

[sherlock-admin3]

Escalate,
I don't agree that there is no impact. As I have mentioned in the impact section of the report, the last users who mint LV tokens via deposits will be stealing funds from the first users. This is not how rewarding contracts work. One user can mint LV tokens in the first round, and if 10 rounds pass and then someone else mints LV tokens, they will receive the same rewards. This is clearly stealing rewards from users. The protocol has confirmed and fixed it, so any claim that this is a design decision is incorrect.

You've created a valid escalation!

To remove the escalation from consideration: Delete your comment.

You may delete or edit your escalation comment anytime before the 48-hour escalation window closes. After that, the escalation becomes final.

[0xsimao]

This is a design choice of the protocol, they do not depend on time to accrue fees, which is fine.

[AtanasDimulski]

@0xsimao I just explained why this is not a design decision. Secondly, reward distributing functionality doesn't work like that. If the HoJ requires more arguments as to why and how this results in users losing funds I will provide them. Let's continue the discussion after the HoJ provides his POV.

[0xsimao]

The root cause in the report is extremely vague

The protocol doesn't implement any mechanism to track when users minted LV tokens, or when they requested a redemption of their LV tokens, or what rewards were accrued to the current LV holders.

The current design of the protocol gives fees to users regardless of the time they spent there, which is fine according to the code and readme, which are the sources of truth.

The readme says

there will likely be external arbitrage bots operating around our system but they are not within scope and act as any market participant

What is happening in these scenarios is that the exchange rate is better than the deposit, but this would never happen due to arbitrage, which is expected as per the readme. So users deposit 1 RA, get 1 LV and redeem 1 LV for more RA.

What would happen with arbitrage is that whenever the pool accrues fees, lps (the lp tokens from the univ2 pool) will be worth more and so will lvs (the liquidity token from the vault). As lvs are minted at 1:1 with the RA deposit, whenever the lv value increases (due to lp fees), it becomes profitable to deposit 1 RA, get 1 LV and redeemEarly(), receiving a part of the fees. In the process of depositing and redeeming early, the exchange rate (lv:ra) will get dilluted back to 1:1, as users redeeming early are getting more ra than what they burn of lv. This process will keep the exchange rate close to 1:1 (minus the feePrecentage of the protocol).

This is a byproduct of minting lv at 1:1 with ra when depositing. If current lv holders want to receive their fees, they can redeem early.

[AtanasDimulski]

@0xsimao so by your logic if I want to provide liquidity by minting LV tokens, for a certain period of time, let's say 10 blocks, and I want to get the maximum amount of fees, I would have to burn and mint LV tokens every single block(if the pair has generated some fees) in order to get the fees that I am owed, this makes no sense. Protocols that distribute rewards based on deposits usually use some variations of this algorithm. I don't see how the possibility of the protocol utilizing arbitrage bots has anything to do with the issue. If it is because of the first POC I have provided, I don't intend to simulate 10 thousand swaps in order to simulate the pair generating fees. The fact that I can deposit exactly after the protocol is deployed and then another user deposits 10 years after that. In the next block, if we both withdraw our LV tokens, we will receive the same amount of fees. This is a definitive loss of funds for the first user, it is not a protocol design (the sponsor has confirmed that this is not the intended behavior), and it is not logical at all. In the readme only the different fee rates are described, nothing else.

[0xsimao]

This is a definitive loss of funds for the first user

But the user can redeem the rewards before to prevent it. This design is suboptimal but it is not loss of funds.

[AtanasDimulski]

It is clearly a loss of funds, let's not continue with the wild speculation that the user is supposed to be frontrunning every other deposit of LV tokens, in order to get the fees he is owed. I don't intend to continue discussing the imaginary scenarios you are presenting that perfectly suit your agenda and are completely incorrect. I will refrain from further comments unless requested by the HoJ, I recommend you do the same, so we don't waste our time.

[WangSecurity]

Firstly, the fact that the sponsor set labels "Sponsor confirmed" and "Will fix" doesn't mean it's not a design decision and the current version of the protocol and may very well mean they just liked the design proposed in the report, i.e. accrue fees based on time.

Secondly, I've got a couple of questions about the report:

The protocol collects fees, from users utilizing its functionality

So, the fees from swaps do not accrue right after the swap, but the protocol has to separately claim them, correct?
In that sense, the users depositing into the protocol to claim fees do not provide any value to the protocol, i.e. it doesn't affect the liquidity of the pool and the price impact, correct?

[0xsimao]

So, the fees from swaps do not accrue right after the swap, but the protocol has to separately claim them, correct?

The fees accrue on the lps of the uni v2 pool, which are held by lv holders. To capture these fees, lv holders need to redeem their lvs to withdraw the corresponding lps from the pool. If the pool had swaps, the lps will be worth more so when lv holders redeem they also get more.

In that sense, the users depositing into the protocol to claim fees do not provide any value to the protocol, i.e. it doesn't affect the liquidity of the pool and the price impact, correct?

If users want to just claim the fees they can deposit and withdraw from the vault, which would slightly reduce the protocol's liquidity (the fees are liquidity on uni v2 pools, there is no distinction between fees or liquidity added, it just redeems pro-rata to the lps burned).

However, in this process if they call VaultLib::redeemEarly(), they also pay a fee. They may not pay a fee if they call VaultLib::redeemExpired(), but the exchange rate may be worse or there may not be liquidity from the previous issuance.

Essentially this design is weird, as the lv value will remain more or less constant over time and users can claim the same revenue indepedently of how long they have been staking.

[AtanasDimulski]

Firstly, the fact that the sponsor set labels "Sponsor confirmed" and "Will fix" doesn't mean it's not a design decision and the current version of the protocol and may very well mean they just liked the design proposed in the report, i.e. accrue fees based on time.

I believe the sponsor confirmed tag and their comment

This is fixed, by implementing an exchange rate for the LV (will add the link to it later)

Clearly indicates that they believe something was wrong with the codebase, and they fixed it, they didn't say we decided to improve it a bit.

As to your question

So, the fees from swaps do not accrue right after the swap, but the protocol has to separately claim them, correct?

I believe @0xsimao has explained it well.

The second question I don't understand clearly and I may provide a bit of a wrong answer, feel free to correct me

In that sense, the users depositing into the protocol to claim fees do not provide any value to the protocol, i.e. it doesn't affect the liquidity of the pool and the price impact, correct?

The purpose of the LV token and why users are incentivized to deposit it, it so that there is a liquidity for a RA:CT UniV2 pair, and there can be trading. UniV2 pairs are extremally dependent on the liquidity within the contract, the less liquidity the bigger the slippage.
Let's take a step back and consider how a UniV2 pair is supposed to work on its own. First liquidity has to be provided for the two tokens if there is one LP provider he will be accruing all the fees. Some time passes the pool is doing well, it is generating fees, a second LP provider comes and deposits liquidity(let's say an equal amount to the first provider so fees should be split 50/50 between them). If they both decide to withdraw at the same time, the first LP provider would get all the fees for the first period where he was the only LP + 50% of the fees for the second period. The second LP will get only 50% of the fees for the second period. For simplicity lets consider in the first period the fees that were generate were 10 tokenX and 10 tokenY, same for the second period. Now the first LP provider will receive 15 tokenX and 15 tokenY, the second LP provider will receive 5 tokenX and 5 tokenY. The first provider had his collateral deposited for a longer period of time where he was taking more risk, for example the price of the token may have dropped, he may have staked it somewhere else and receive more rewards for example ETH may be staked for stEHT. The possibility of impermanent loss should also be considered. I don't believe this should be considered as an opportunity loss, because the problem of misdistribution of fees comes from the logic of the Cork implementation.

However in the case of the Cork protocol if we have the same scenario as described above both of the LPs (people who mint the LV token) will receive 10 tokenX and 10 tokenY. This is clearly a theft of funds from the first LP provider. If for example I have been providing liquidity for 30 days and accruing fees, someone deposits the same amount of LV tokens, and then in the next block we both redeem our LV tokens we will both receive the same amount of fees. The second depositor have risked his collateral for a total of 2 blocks, and receives the same amount of fees as the first who risked his collateral for 30 days and did a great favor to the protocol by providing liquidity to the pair.

Secondly the LV token is the same for a RA:PA pair, but the DS and CT tokens have an expiration, and multiple DS and CT tokens can be minted, thus different RA:CT uniV2 pairs can exist. When DS and CT tokens expire, the LP owned by the protocol in the pair is liquidated, the CT is converted to RA, then a new uniV2 pair is created for the newly issued CT token, the RA is split between RA and the new CT token and deposited in the new uniV2 pair. The protocol generates fees from the redeemRaWithDS() and repurchase() functions as well, the fee is split between RA and CT and deposited in the uniV2 pair. Now if we have the same case as above someone mints LV at the first block, users utilize the protocol and protocol fees are generated, lets say 10 RA and another user mints LV at the last block, when the first DS and CT tokens expire and a new RA:CT2 pair is created all those fees will be mashed together, and again the second user will profit tremendously.

[WangSecurity]

Thank you for these great explanations. I'll re-iterate to confirm I understand everything correctly:

When we deposit into the pool, we basically add liquidity for this RA/PA pair on UniV2 which improves the slippage. So let's observe the attack in this report with an example:
One user wants to initiate a very large swap, let's say 1M USDC volume. The attacker sees it and deposits into Cork. They get a portion of the fees, but they also contributed to that swap and improved slippage, i.e. e.g. without the attacker depositing the user would lose 50K due to slippage and price impact, but with the attacker deposit, they've ended with only 1K slippage and received 999K USDC in the end.

So while the attacker received the portion of the fee, they also contributed to the pool and made slippage better. Is the example above correct? As I understand it wouldn't be possible to also amplify flash loans, since the attacker would be able to withdraw only in the next block?

[AtanasDimulski]

The uniV2 pair that the protocol creates is for RA for example ETH, and a CT token that the protocol creates and controls the mining of. The PA can be stETH for example which is supposed to be pegged to RA. The CT token has a different purpose within the protocol than the PA token. Users who mint LV tokens deposit RA token into the Cork protocol, which is split between RA and CT and is fully deposited into the uniV2 pair, we can consider them as LPs. If someone frontruns a big swap and deposits to the pair he should also receive fees from that big swap. Problem is if there is 1 big swap and lets say only user A has provided liquidity, the pair generates 1k fees, after the swap user B provides liquidity by minting LV tokens, then both user A and user B redeem their LV tokens they will both receive 500 in fees. The vulnerability I tried to describe is not about a depositor frontruning big swaps and generating the same fees, but more like depositing after the big swap, not providing any liquidity that helps with slippage for said big swap, and receiving the same fees as the depositor who provided the only liquidity before the big swap. If only User A has provided liquidity before the big swap that generates 1k in fees, he should receive the whole fee, however this is not the case.

[WangSecurity]

Oh, I see, so it returns a bit to my comment before it. The fees are generated not in the same transaction as the swap. The fees are then claimed separately, and the attacker can deposit just to receive the fees without providing liquidity for that big swap, for example, correct?

[AtanasDimulski]

the attacker can deposit just to receive the fees without providing liquidity

This part is correct, and it doesn't have to be an attacker, just a user depositing at some random time. This is simply how the protocol works.
A bit of clarification on the fee generation part. The fees are accrued to the LP tokens in the same transaction as the swap. In the case of uniV2 LP tokens are like shares. UniV2 is like ERC4626 vault. Users have to burn their LP tokens in order to get back their liquidity + fees. Say there is 100 liquidity and 10 LP shares, after the swap there is 110 liquidity and still 10 LP shares, now if the LP burns his shares he gets 110 liquidity - 100 the original liquidity he deposited + 10 from the generated fees. 1 LP share is now worth 11 liquidity. The redemption in this case will be (10 * 110) / 10 = 110. If the first user hasn't redeemed, the swap has happened and then another user comes in and deposits another 100 liquidity his shares will be calculated in the following way (100 * 10) / 110 ~ 9, now if they both redeem at the same time, with no other fees user A will get for his 10 shares (10 * 210) / 19 ~ 110, user B for his 9 shares will get (9 * 100) / 9 = 100. However, since the Cork protocol owns the LP tokens or the shares in the above example. The same steps happen as above, user A and user B now own 1 LV token each. The protocol owns 19 LP shares, but each 1 LV token is worth 19/2 = 9,5 shares. Thus when the LV are redeem user A and user B will receive the same amount of fees.

[WangSecurity]

Thank you very much for this explanation. I think I've got the very last question to make the decision:
For this attack, the attacker doesn't depend on large swaps, and they can wait for lots of small swaps to happen, so Cork protocol accrues fees and then executes the attack, correct?

[AtanasDimulski]

Yes UniV2 swap fees are 0.3% for each swap, the attacker can wait for a month for example for thousands of small fees to occur, and then provide liquidity, the result will be the same.

[WangSecurity]

Thank you again for the clarification. I agree this is a valid finding, the attacker doesn't contribute to the protocol and would receive the fees even for swaps they didn't provide liquidity for. This can be viewed as a design decision, but this is a loss of funds (in this case, fees), so valid finding.

About the constraints, the one I see here is paying the early redemption fee, but the attacker can just wait until the fees stack up and exceed the early redemption fee. The second one is the attacker would need a relatively large capital to get a large part of the fee. I don't see them as extensive limitations. Hence, high severity should be appropriate.

Planning to accept the escalation and validate with high severity. Are there any duplicates?

[AtanasDimulski]

Hey, @WangSecurity I haven't checked extensively, but @cvetanovv didn't group this finding with anything else + I haven't seen something similar being escalated.

[WangSecurity]

Result:
High
Unique

[sherlock-admin4]

Escalations have been resolved successfully!

Escalation status:

• AtanasDimulski: accepted