0x73696d616f - Missing slippage check for rewards in `ZivoeTranches::depositJunior()` and `ZivoeTranches::depositSenior()`

[sherlock-admin3]

0x73696d616f

high

Missing slippage check for rewards in ZivoeTranches::depositJunior() and ZivoeTranches::depositSenior()

Summary

In ZivoeTranches::depositJunior() and ZivoeTranches::depositSenior() users receive rewards which depend on the ratio between juniorSupp and seniorSupp. Additionally, the contract only gives rewards up to its ZVE balance. Thus, users may submit a transaction expecting to receive a certain amount, but be frontrunned and receive 0 instead.

Vulnerability Detail

In ZivoeTranches::rewardZVEJuniorDeposit() and ZivoeTranches::rewardZVESeniorDeposit(), ZVE rewards are calculated according to reward = avgRate * deposit / 1 ether;, where avgRate is the average between the juniorSupp * BIPS / seniorSupp; ratios before and after the deposit. This means that users can expect to receive a certain amount of rewards in the frontend, but receive much less in reality due to having been frontrunned. In fact, the rewards can even be 0 in case there are not enough ZVE due to a frontrunning transaction.

Add the following test to Test_ZivoeTranches.sol confirming that jim was frontrunned and got 0 rewards.

function test_POC_ZivoeTranches_depositSenior_noSlippageCheck() public {
    uint256 maximumAmount_18 = 1000 ether;
    simulateITO(100_000_000 ether, 100_000_000 ether, 100_000_000 * USD, 100_000_000 * USD);

    assert(god.try_updateMaxZVEPerJTTMint(address(ZVT), 0.4e18));
    assert(god.try_updateMinZVEPerJTTMint(address(ZVT), 0.3e18));

    // rewards are bigger than 0, jim seems this in the frontend but is frontrunned by sam
    uint256 samPreRewardsBalance = IERC20(address(ZVE)).balanceOf(address(sam));
    mint("DAI", address(sam), 10_000_000_000 ether);
    assert(sam.try_approveToken(address(DAI), address(ZVT), 10_000_000_000 ether));
    vm.prank(address(sam));
    ZVT.depositSenior(maximumAmount_18, address(DAI));
    assertGt(IERC20(address(ZVE)).balanceOf(address(sam)), samPreRewardsBalance);

    // Jim gets 0 rewards but expected to get much more
    uint256 jimPreRewardsBalance = IERC20(address(ZVE)).balanceOf(address(sam));
    mint("DAI", address(jim), maximumAmount_18);
    assert(jim.try_approveToken(address(DAI), address(ZVT), maximumAmount_18));
    vm.prank(address(jim));
    ZVT.depositSenior(maximumAmount_18, address(DAI));
    assertEq(IERC20(address(ZVE)).balanceOf(address(jim)), jimPreRewardsBalance);
}

Impact

User receives much less rewards than expected due to being frontrunned.

Code Snippet

ZivoeTranches::depositJunior()

function depositJunior(uint256 amount, address asset) public notPaused nonReentrant {
    ...    
}

ZivoeTranches::depositSenior()

function depositSenior(uint256 amount, address asset) public notPaused nonReentrant {
    ...    
}

Tool used

Manual Review

Vscode

Foundry

Recommendation

Add slippage check arguments for the rewards given in ZivoeTranches::depositJunior() and ZivoeTranches::depositSenior().

...
function depositJunior(uint256 amount, address asset, uint256 minRewards) public notPaused nonReentrant {
    ...
    require(incentives >= minRewards, "ZivoeTranches::depositJunior() incentives < minRewards");
}
...
function depositSenior(uint256 amount, address asset, uint256 minRewards) public notPaused nonReentrant {
    ...    
    require(incentives >= minRewards, "ZivoeTranches::depositSenior() incentives < minRewards");
}
...

Duplicate of #63

[sherlock-admin2]

1 comment(s) were left on this issue during the judging contest.

panprog commented:

borderline low/medium, dup of #63, the core issue here is that incentive per token deposited can change before user's transaction executes (for different reasons) and user has no control (slippage check) about it, but this is a protocol choice and this issue can be considered informational. Still, the incentive might be important to user, hence borderline

[0x73696d616f]

escalate

this issue is exactly like inflation attacks, accepted in previous contests as valid. Rewards are being stolen, as the user has no control over how much it will receive. It should be a valid medium.

[sherlock-admin3]

escalate

this issue is exactly like inflation attacks, accepted in previous contests as valid. Rewards are being stolen, as the user has no control over how much it will receive. It should be a valid medium.

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.

[0xDenzi]

@panprog #683 mentions the right impact of this issue, With the loss of rewards already occurring on the user whose transaction is executed second combined with the fact that Junior tranche position holders are main frontline to bear losses due to defaults makes this a valid high imo.

Suppose Person A wants to deposit into the junior tranche, they accept that If there are any defaults then they'll have to bear the losses through their deposit but for this they would like to have the max incentives so at least they are getting a good amount of rewards back for the risk they are taking. They prepare their transaction such that they receive the max rewards for their deposit but they get front ran by Person B. Person A's transaction will execute later which will cause them to not receive the max rewards + their deposit is at risk due to defaults. Now to get back their deposit since they do not like the rewards they are getting back in comparison to the risk they are taking, they will have to wait for the next epoch. During this time there is a good chance a default can occur which will make them lose a portion or all of the deposit (if the default is big enough). There are other conditions while redeeming trying to redeem as well such as

As epoch 2 starts, they can begin redeeming their tokens. However, due to the limited available capital (50,000 USDC), and a total outstanding request size of 200,000 tranche tokens, each user can redeem a maximum of 25,000 USDC. In subsequent epochs, if more capital becomes available, more tranche tokens can be redeemed.

So Person A might have to wait even longer to exit their undesirable position exposing it to further risk for prolong period.

Redemptions docs give this a read for more context.

I believe that this is a valid concern for users who want to enter into a position, they should be getting their desired reward for the risk they are taking. Introducing a slippage check on incentives makes this secure as the transaction will simply revert if the reward they were desiring is not possible anymore.

[panprog]

This is borderline issue, so I see both arguments for and against it. Still, on the balance of everything my view is that this is more low than medium as I've never seen slippage control for incentive token, it's supposed to be something extra, not the main thing of why users deposit, especially given the rather long lock time before the withdrawals are opened.

[0x73696d616f]

The fact that incentives may go to 0 or decrease significantly instantly should award this medium classification.

[0xDenzi]

The slippage is required here so that a person who wants to obtain a position in the tranche is not susceptible to grieifing intentionally or unintentionally, furthermore the reward is the ZVE token which can be staked to obtain USDC Docs.

The issue identifies loss of funds/rewards for the depositor, combined with the fact that the position can not be exited until a time period should be considered as high. It's not a matter of what is the norm for incentive tokens as the purpose and usage of them can vary, In this protocol, The depositor trusts the system and takes the risk (incase of defaults) for the ZVE and Tranche Tokens they are receiving back which can be staked to generate USDC or governance purposes. A person might be willing to take the risk in return for max incentives but not for subpar incentives but once they are assigned subpar incentives, they are stuck with it until they can redeem back their deposit.

According to sherlock docs.

Slippage related issues showing a definite loss of funds with a detailed explanation for the same can be considered valid high

[0x73696d616f]

especially given the rather long lock time before the withdrawals are opened

I think this line of reasoning misses the fact that users may buy the tranches just for the incentives and sell them immediately.

[WangSecurity]

Firstly, I agree that the users can receive less ZVE (down to 0), but the problem is that the user is not required to be front-run and it is what may happen in the regular workflow of the protocol. If we look at the end of both rewardZVESeniorDeposit and rewardZVEJuniorDeposit, the intentionally decrease the amount of incentives in ZVE if there is not much token in the contract. At this point there might be no ZVE token cause of 25M tokens hard cap, therefore, I believe it's an intended design.

Moreover, the users don't lose all the rewards, they still get zJTT/zSTT. Hence, I believe it has to remain invalid as an intended design cause it's not guaranteed there will always be enough ZVE as rewards for tranche deposits, planning to reject the escalation and leave the issue as it is.

[0x73696d616f]

@WangSecurity I am commenting by no means to dispute your decision, just making sure we have all the facts.

The problem with frontrunning is that it is never intended for the protocol. In this case, a user may see in the frontend that he will get 1000 rewards tokens, deposit into the tranche, but receive 0 rewards later. This will disincentivize this user and other users to deposit into tranches as they have been exploited.

I also agree that not giving rewards tokens is intended when there are none, it is not the point of this issue. But the user should be protected from being frontrunned and the rewards becoming 0 in the meantime. Example: user sees in the frontend there are 1000 rewards to claim, sends deposit into tranch transaction, but someone frontruns the user and grabs the 1000 rewards. Now the user gets 0 instead of 1000.

Lastly users do lose all their rewards, the zJTT/zSTT they get are not rewards, but the legitimate tokens received due to giving collateral.

So essentially the point of the issue is not that it is incorrect to receive 0 rewards (nor reduced rewards), but users need to be able to do something about it, namely by setting a slippage parameter.

[0xDenzi]

@WangSecurity

Moreover, the users don't lose all the rewards, they still get zJTT/zSTT. Hence, I believe it has to remain invalid as an intended design cause it's not guaranteed there will always be enough ZVE as rewards for tranche deposits

The tranche tokens are minted in 1:1 ratio to the deposit of the user, but the ZVE tokens are the rewards, both of them have value and can be used to generate more value in the form of staking them back into the system or sell them. The user is losing value in this scenario mentioned by not being protected by a slippage parameter.

A user might want to deposit for max rewards and not for any amount less than that. If they are front ran by another user, their transaction will still go through and they will be assigned lower than max.

As mentioned by @0x73696d616f

user sees in the frontend there are 1000 rewards to claim, sends deposit into tranch transaction, but someone frontruns the user and grabs the 1000 rewards. Now the user gets 0 instead of 1000.

This is a major issue imo and not intended design. The issue clearly describes loss of funds for the user for not being protected by a slippage parameter.

[0xDenzi]

@WangSecurity This comment shows how much loss of rewards occur if a user gets front-ran

Consider the following values

• maxZVEPerJTTMint: 0.45 ZVE per stablecoin
• minZVEPerJTTMint: 0.05 ZVE per stablecoin
• lowerRatioIncentiveBIPS: 1000 BIPS (10%)
• upperRatioIncentiveBIPS: 3500 BIPS (35%)
• seniorSupp: 100,000 stablecoins
• juniorSupp: 2,000 stablecoins
• BIPS: 10,000 for calculating percentages

The current supply values mean that the junior supply can be increased by deposits of <= 8000 stablecoins to receive max rewards, If the next deposit is 8000, This will place the junior tranche ratio to senior exactly at 10%, After which any deposits made will not receive max rewards but instead will be calculated a new avgRate depending on the supply and ratio.

Bob sees the current difference in supplies, calculates (or sees on frontend) how many ZVE rewards a deposit of 8000 stablecoins, i.e which is exactly reward= avgRate × deposit = 0.45 × 8,000 = 3600 ZVE. He creates the transaction from his end in order to receive 8000 junior tranche tokens(1:1 to deposit) and 3600 ZVE Tokens.

Now Another user suppose Alice creates her own transaction with higher gas fees, and fills up the supply of the junior tranche and makes it equal to the lowerRatioIncentiveBIPS i.e 10%. If Bob's transaction gets executed now, he will not receive the rewards he had previously calculated but instead receive much lower, the new avgRate will be calculated as 0.386 in this scenario and the rewards bob will receive are now equal to reward= avgRate × deposit = 0.386 × 8,000= 3088 ZVE

So now the protocol will accept 8000 stablecoins from Bob and also return him less Rewards for the risk he is taking. This is a major issue as Bob is misled, Consider a scenario where Bob only wanted to deposit if he was receiving the max rewards but now his funds are stuck in the system until the next epoch for redemptions. If there is a slippage on the rewards the user is expecting back, then this scenario will not occur.

P.S maxZVEPerJTTMint will always be less than 0.5 as there is a require check in updateMaxZVEPerJTTMint().

[WangSecurity]

I agree that this situation may happen that a user can get front-run, even unintentionally, by another user and receive 0 or less rewards than they expected. But, since it's design decision there will be actually 0 rewards in ZVE I believe it's also a design decision that the protocol acts as a first come, first served when it comes to rewards and if the user wants to get them, they could deposit earlier. Moreover, users don't lose tranche tokens which can be used to gain value (rewards from staking) from the protocol, and only lose an opportunity earn more rewards. Lastly, manipulating/front-running the incentives is costly and doesn't give any significant profit for the attacker (unless there are not much incentives).

Hence, I believe this report should remain invalid, planning to reject the escalation.

[0xDenzi]

@WangSecurity I agree with you that its design decision that at one point there will be 0 ZVE rewards in the tranche, Although that situation will be different, Users will be depositing knowing that they will only receive the tranche tokens and 0 rewards.

In the current state which the issue is describing users are calculating the rewards they are going to get and then proceeding with their transaction. Depositing earlier is not a mitigation to front-running as the likelihood to be front ran is still possible at any moment.

I also agree with the design of first come, first serve but this design needs the slippage protection to ensure that users who actually come first do not lose value due to a front-run, once the transaction reverts due to slippage, they can always recalculate the rewards again and proceed with their transaction again if they want to for less rewards.

In my comment above, the front runner has a big incentive of around 500+ reward tokens, which were supposed to be given to Bob. These reward tokens can be staked to gain more value so a big incentive is present for frontrunners.

[WangSecurity]

I understand that scenario, but we have to remember that this issue may only happen if there are not many incentives left or the attacker spends a lot of funds on this attacker (so incentives left < incentives the user should get). But as I've said this is the design to allow such behaiour and pay out the tokens to the ones who deposits earlier. The same can happen if simply two users deposit in the same block and only one gets the reward. Moreover, the user doesn't lose their funds, they can withdraw them after a timelock. Hence, I believe it's design decision and the report should remain invalid. Planning to reject the escalation and leave the issue as it is.

[0xDenzi]

@WangSecurity Thank you for continuing this long tiring discussion but i must add to the following comments

The same can happen if simply two users deposit in the same block and only one gets the reward.

I believe this warrants a slippage protection, reverting the 2nd deposit and giving the user their choice to deposit again is the right choice or else they will lose trust in the system for not receiving the rewards they were expecting.

Moreover, the user doesn't lose their funds, they can withdraw them after a timelock.

Again I believe this is another reason for slippage as users who will calculate higher rewards and proceed with their deposit should be given their funds back by reverting the call if the amount of rewards decrease

Users are locking their funds in the system for the combined value of rewards(they calculated or saw on frontend) + tranche tokens, the funds locked are going to get hit by defaults so they are taking a risk on depositing in exchange for amounts of both tokens they expect to receive.

Locking their funds and not giving back them their calculated amounts, and then if a default occurs will make them get even less value out when they withdraw their amount.

[WangSecurity]

Firstly, users take risk of defaults if they deposit in the protocol in the first place, regardless of what rewards they get. Secondly, the users know that their funds will be locked and it won't an unexpected DOS. Moreover, the users still are able to gain value from the protocol from staking tranche tokens. I agree that the slippage protection in that case would be good, but I see it as a design improvement rather than the security issue. Decision, remains the same, reject the escalation and leave the issue as it is.

[0xDenzi]

@WangSecurity I think I did not use the right words, I'll correct

Users know their funds are going to get locked and defaults will hit their funds 'in exchange for the Tranche and Rewards Tokens.'

Consider a user who made up their mind to take the risk of depositing 8000 stablecoins in return for 8000 tranche tokens and 3600 ZVE tokens, Consider the combined value that he is expecting back on his deposit but they get front ran and receive 8000 tranche tokens and 3100 ZVE tokens, the combined value is lower and not what they expected and yet their funds are locked, that user might not have wanted to lock their funds and that risk for this lower combined value.

When it comes to staking, staking this combined value will net them less value than the combined value of what he had calculated to receive so he still losing value when staking.

[0x73696d616f]

@omar-ahsan I think we have made our points clear, please do not add the same information over and over again.

[WangSecurity]

Yep, this points were expressed above already and were taken into consideration for the final decision.

[0xDenzi]

@0x73696d616f Not adding anymore, just thought this was missed and needed emphasis.

@WangSecurity thank you for taking your time to reply to each argument.

[Evert0x]

Result: Invalid Duplicate of #63

[sherlock-admin2]

Escalations have been resolved successfully!

Escalation status:

• 0x73696d616f: rejected