samuraii77 - Forwarding yield in `OCL_ZVE` is possible a lot more often than the enforced 30 days

[sherlock-admin2]

samuraii77

medium

Forwarding yield in OCL_ZVE is possible a lot more often than the enforced 30 days

Summary

Calling OCL_ZVE::forwardYield() is supposed to only be possible every 30 days however in reality, it is possible to call the function a lot more often.

Vulnerability Detail

The require statement in OCL_ZVE::forwardYield() enforces that the function should only be callable when block.timestamp is of larger value than the value of nextYieldDistribution

    function forwardYield() external {
        if (IZivoeGlobals_OCL_ZVE(GBL).isKeeper(_msgSender())) {
            require(
                block.timestamp > nextYieldDistribution - 12 hours, 
                "OCL_ZVE::forwardYield() block.timestamp <= nextYieldDistribution - 12 hours"
            );
        }
        else {
            require(
                block.timestamp > nextYieldDistribution, 
                "OCL_ZVE::forwardYield() block.timestamp <= nextYieldDistribution"
            );
        }

        (uint256 amount, uint256 lp) = fetchBasis();
        if (amount > basis) { _forwardYield(amount, lp); }
        (basis,) = fetchBasis();
        nextYieldDistribution += 30 days;
    }

Then, at the end of the function, nextYieldDistribution is incremented by 30 days making the function supposedly not callable for another 30 days. However, there is a vulnerability that allows OCL_ZVE::forwardYield() to be called a lot more times.

Upon calling the OCL_ZVE::pushToLockerMulti(), the nextYieldDistribution gets set to block.timestamp + 30 days if its value is 0.

if (nextYieldDistribution == 0) { nextYieldDistribution = block.timestamp + 30 days; }

If this is the way nextYieldDistribution gets its first value, then there will not be an issue. However, if OCL_ZVE::forwardYield() is called beforehand, then nextYieldDistribution will be set to 0 += 30 days which equals 30 days making its value a lot less than the value of block.timestamp resulting in people being able to call OCL_ZVE::forwardYield() at will all the way until nextYieldDistribution gets incremented all the way to block.timestamp.

Calling OCL_ZVE::forwardYield() before any other function is possible and requires just 1 simple circumstance to be a fact.

Imagine the following scenario:

1. OCL_ZVE::forwardYield() is called
2. The if statement passes as block.timestamp is larger than nextYieldDistribution, the value of which is the default value of 0
3. OCL_ZVE::fetchBasis() gets called

   function fetchBasis() public view returns (uint256 amount, uint256 lp) {
       address pool = IFactory_OCL_ZVE(factory).getPair(pairAsset, IZivoeGlobals_OCL_ZVE(GBL).ZVE());
       uint256 pairAssetBalance = IERC20(pairAsset).balanceOf(pool);
       uint256 poolTotalSupply = IERC20(pool).totalSupply();
       lp = IERC20(pool).balanceOf(address(this));
       amount = lp * pairAssetBalance / poolTotalSupply;
   } 

4. As long as there is a pool setup for pairAsset and ZVE, the vulnerability will take place
5. Pool gets the value of the pool address
6. The only thing which has to pass here is poolTotalSupply not being 0 as division by 0 is not possible
7. That successfully passes as there is already a setup pool for pairAsset and ZVE
8. Then, we get back to the OCL_ZVE::forwardYield() function
9. if(amount > basis) does not pass as both values are 0
10. On the last line, nextYieldDistribution gets set to 30 days making the OCL_ZVE::forwardYield() function callable again and again

Impact

OCL_ZVE::forwardYield() is callable over and over again even though it is only supposed to be called every 30 days

Proof Of Concept

Paste the following test into Test_OCL_ZVE.sol:

function testCanForwardYieldALot() public {
        address UNIV2_ROUTER = OCL_ZVE_UNIV2_DAI.router();
        deal(DAI, address(this), 10000);

        IERC20(DAI).safeApprove(UNIV2_ROUTER, 10000);
        IERC20(ZVE).safeApprove(UNIV2_ROUTER, 1001);

        IUniswapV2Router01(UNIV2_ROUTER).addLiquidity(address(DAI), address(ZVE), 10000, 1001, 0, 0, address(this), block.timestamp);

        uint256 count;
        while (block.timestamp > OCL_ZVE_UNIV2_DAI.nextYieldDistribution()) {
            OCL_ZVE_UNIV2_DAI.forwardYield();
            count++;
        }

        console.log(count);
    }

Code Snippet

https://github.com/sherlock-audit/2024-03-zivoe/blob/d4111645b19a1ad3ccc899bea073b6f19be04ccd/zivoe-core-foundry/src/lockers/OCL/OCL_ZVE.sol#L186 https://github.com/sherlock-audit/2024-03-zivoe/blob/d4111645b19a1ad3ccc899bea073b6f19be04ccd/zivoe-core-foundry/src/lockers/OCL/OCL_ZVE.sol#L287-L305

Tool used

Manual Review

Recommendation

Do not allow OCL_ZVE::forwardYield() to be called whenever the value of nextYieldDistribution is equal to 0. Also, another option is to set nextYieldDistribution to block.timestamp + 30 days instead of just incrementing it by 30 days.

[coreggon11]

If pushToLockerMulti was not called, then nothing will be forwarded.

[sherlock-admin3]

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

panprog commented:

medium, if forwardYield is called just after contract creation, then nextYieldDistribution becomes incorrect and forwardYield can be called at any time by anyone. The impact is limited, but the protocol functionality is broken (no time limit for calling forwardYield)

[pseudonaut]

Wouldn't _forwardYield() revert due to division by 0 if basis is 0 ?

function forwardYield() external {
        ...
        (uint256 amount, uint256 lp) = fetchBasis();
        if (amount > basis) { _forwardYield(amount, lp); }

Or rather _forwardYield() wouldn't even be called if (amount > basis) { _forwardYield(amount, lp); } ...

function _forwardYield(uint256 amount, uint256 lp) private nonReentrant {
        address ZVE = IZivoeGlobals_OCL_ZVE(GBL).ZVE();
        uint256 lpBurnable = (amount - basis) * lp / amount * (BIPS - compoundingRateBIPS) / BIPS;

[panprog]

@pseudonaut This is correct, _forwardYield will not be called, the point is that the last line of forwardYield:

nextYieldDistribution += 30 days;

will make nextYieldDistribution = 30 days, thus all future forwardYield will ignore timestamp check because block.timestamp will always be greater than nextYieldDistribution.

The issue is valid, keeping it medium.

[spacegliderrrr]

Escalate

Issues which can be mitigated by correct admin configuration upon deployment are not valid according to Sherlock rules. Issue should be low.

[sherlock-admin3]

Escalate

Issues which can be mitigated by correct admin configuration upon deployment are not valid according to Sherlock rules. Issue should be low.

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.

[samuraii77]

Escalate

Issues which can be mitigated by correct admin configuration upon deployment are not valid according to Sherlock rules. Issue should be low.

Pushing into the locker upon deployment is in no way, shape or form, correct admin configuration. That is not even a configuration.

[panprog]

Agree with @samuraii77 , this can only be mitigated if admin pushes into locker in the same transaction as contract creation, which doesn't seem to be reasonable assumption as this should not be part of the deployment.

[sherlock-admin2]

The protocol team fixed this issue in the following PRs/commits: https://github.com/Zivoe/zivoe-core-foundry/pull/265

[WangSecurity]

As I understand from Zivoe's Deployment diagram on Figma, they indeed will initially call push on lockers (phase three) and only then forwardYield (phase four). Hence, I believe this report requires an admin acting as not intended (i.e. calling forwardYield before pushToLockerMulti), therefore, should be invalid.

Planning to reject an escalation and leave the issue as it is.

[samuraii77]

The issue does not rely on an admin acting not as intended. The forwardYield() function can be called by any user after the contract has been deployed which as visible by the diagram you provided happens at phase 1 and pushing happens all the way in phase 3. Basically, that means that a user has the time between phase 1 and phase 3 to just call forwardYield() in order for the vulnerability to happen. Issue should stay valid.

Also, I am not sure if I get you correctly but it seems like you are disputing my issue and agreeing with the escalation but then proceed to say that you will reject the escalation and keep the issue as it is, not sure if I interpreted your comment wrong.

[WangSecurity]

Yep, sorry for that two confusions here and I agree that indeed the locker is deployed earlier than push is called. Therefore, this issue indeed may occur.

Planning to reject the escalation and leave the issue as it is.

[Evert0x]

Result: Medium Has Duplicates

[sherlock-admin4]

Escalations have been resolved successfully!

Escalation status:

• spacegliderrrr: rejected

[sherlock-admin2]

The Lead Senior Watson signed off on the fix.