No slippage for ZVE rewards when depositing to tranches.

Summary

Whenever there is an exchange of value or reward transfer, there should be a minimum slippage input parameter because the amount of reward/tokenOut you will receive depends on a lot of states that are manipulatable by MEV or genuine normal transactions. Rewards that a user will receive when depositing is vulnerable to many changes in state that can be triggered by MEV or genuine normal transactions.

Vulnerability Detail

https://github.com/sherlock-audit/2024-03-zivoe/blob/01e00e6f27b58392a6fa0b82c84a46a783a0df3c/zivoe-core-foundry/src/ZivoeTranches.sol#L236


File: 2024-03-zivoe\zivoe-core-foundry\src\ZivoeTranches.sol
238:     function rewardZVESeniorDeposit(uint256 deposit) public view returns (uint256 reward) {
239: 
240:         (uint256 seniorSupp, uint256 juniorSupp) = IZivoeGlobals_ZivoeTranches(GBL).adjustedSupplies();
241: 
242:         uint256 avgRate;    // The avg ZVE per stablecoin deposit reward, used for reward calculation.
243: 
244:  >>>    uint256 diffRate = maxZVEPerJTTMint - minZVEPerJTTMint;
245: 
246:         uint256 startRatio = juniorSupp * BIPS / seniorSupp;
247:         uint256 finalRatio = juniorSupp * BIPS / (seniorSupp + deposit);
248:  >>>    uint256 avgRatio = (startRatio + finalRatio) / 2;
249: 
250:         if (avgRatio <= lowerRatioIncentiveBIPS) {
251:             avgRate = minZVEPerJTTMint;
252:         } else if (avgRatio >= upperRatioIncentiveBIPS) {
253:             avgRate = maxZVEPerJTTMint;
254:         } else {
255:  >>>        avgRate = minZVEPerJTTMint + diffRate * (avgRatio - lowerRatioIncentiveBIPS) / (upperRatioIncentiveBIPS - lowerRatioIncentiveBIPS);
256:         }
257: 
258:         reward = avgRate * deposit / 1 ether;
259: 
260:         // Reduce if ZVE balance < reward.
261:         if (IERC20(IZivoeGlobals_ZivoeTranches(GBL).ZVE()).balanceOf(address(this)) < reward) {
262:  >>>        reward = IERC20(IZivoeGlobals_ZivoeTranches(GBL).ZVE()).balanceOf(address(this));
263:         }
264:     }

Users can deposit to tranches by calling ZivoeTranches.depositJunior() and ZivoeTranches.depositSenior(). And rewards will be incentivized in ZVE tokens depending on the following parameters: 1. juniorSupp and seniorSupp 2. if avgRatio > or < than their lowerRatioIncentiveBIPS or upperRatioIncentiveBIPS 3. diffRate which depends on maxZVEPerJTTMint and minZVEPerJTTMint 4. Balance of ZVE tokens inside the ZivoeTranches.sol

With this much dependence and calculation in determining how much to reward to current tranche deposit based on the tranche supplies, zve balance, the ratios and minting rates as shown above. This shows that perople will deposit to tranches in a hope of getting these rewards. But look at the attack path.

attack path:

A user calls ZivoeTranches.depositSenior(), by seeing how much rewards he's will receive if he deposits 1000 $ stable coins might get 20 ZVE depending on the minting rate.
But someone frontruns this deposits and make a huge deposit to the same tranche which will increase the tranche token supply, if supply increases it will decrease avgRate in L255 above, which will decrease the amount of rewards he gonna get.

So in this way, a genuine front run might also happen, which can decrease the total supply of the tokens and it will increase amount of rewards frontrunner will receive. But rewards are so low, then L262 will trigger for the victim's deposit and he will get less rewards.

And owner might change max and minimum minting rate, which will also affect how many rewards the victim will receive if he gets frontran.

So to deal and not hurt how much rewards the user will receive, mention a slippage parameter on ZivoeTranches.depositJunior() and ZivoeTranches.depositSenior().

Impact

Loss of ZVE rewards due to MEV, or genuine change in minting rate, JTT/STT token total supplies, and tranche ratios change. So giving it a medium

Code Snippet

Tool used

Manual Review

Recommendation

slippage parameter on ZivoeTranches.depositJunior() and ZivoeTranches.depositSenior().


-   function depositSenior(uint256 amount, address asset) public notPaused nonReentrant {
+   function depositSenior(uint256 amount, address asset, uint minReward) public notPaused nonReentrant {
        require(
            IZivoeGlobals_ZivoeTranches(GBL).stablecoinWhitelist(asset), 
            "ZivoeTranches::depositSenior() !IZivoeGlobals_ZivoeTranches(GBL).stablecoinWhitelist(asset)"
        );
        require(tranchesUnlocked, "ZivoeTranches::depositSenior() !tranchesUnlocked");

        address depositor = _msgSender();

        IERC20(asset).safeTransferFrom(depositor, IZivoeGlobals_ZivoeTranches(GBL).DAO(), amount);
        
        uint256 convertedAmount = IZivoeGlobals_ZivoeTranches(GBL).standardize(amount, asset);

        uint256 incentives = rewardZVESeniorDeposit(convertedAmount);
+      require(incentives > minReward, "low rewards");

        emit SeniorDeposit(depositor, asset, amount, incentives);

        // Ordering important, transfer ZVE rewards prior to minting zJTT() due to totalSupply() changes.
        IERC20(IZivoeGlobals_ZivoeTranches(GBL).ZVE()).safeTransfer(depositor, incentives);
        IERC20Mintable_ZivoeTranches(IZivoeGlobals_ZivoeTranches(GBL).zSTT()).mint(depositor, convertedAmount);
    }

Duplicate of #63

sherlock-admin3 commented 6 months ago

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

pseudonaut commented 6 months ago

Front-running should not be a valid issue as people can use flash-bots RPC to make calls through Metamask

sherlock-audit / 2024-03-zivoe-judging

Ironsidesec - No slippage for ZVE rewards when depositing to tranches. #353