Yield in `updateYield` is not always updated as it should be

[c4-bot-3]

Lines of code

https://github.com/code-423n4/2024-03-dittoeth/blob/91faf46078bb6fe8ce9f55bcb717e5d2d302d22e/contracts/libraries/LibVault.sol#L99

Vulnerability details

Impact

1. Yield in updateYield is not always updated (inaccurate).
2. TAPP.ethEscrowed is not up to date.
3. Vault.dethTotal is not up to date, _ethConversion would return wrong amount in withdraw.

Proof of Concept

if (dethYieldRate == 0) return this return is intentional, since we don't have dethYieldRate, no need to update Vault.dethYieldRate. However we have a case that could cause Vault.dethTotal not to be up to date.

    function updateYield(uint256 vault) internal {
        AppStorage storage s = appStorage();

        STypes.Vault storage Vault = s.vault[vault];
        STypes.VaultUser storage TAPP = s.vaultUser[vault][address(this)];
        // Retrieve vault variables
        uint88 dethTotalNew = uint88(getDethTotal(vault)); // @dev(safe-cast)
        uint88 dethTotal = Vault.dethTotal;
        uint88 dethCollateral = Vault.dethCollateral;
        uint88 dethTreasury = TAPP.ethEscrowed;

        // Pass yield if > 0
        if (dethTotalNew <= dethTotal) return;
        uint88 yield = dethTotalNew - dethTotal;

        // If no short records, yield goes to treasury
        if (dethCollateral == 0) {
            TAPP.ethEscrowed += yield;
            Vault.dethTotal = dethTotalNew;
            return;
        }

        // Assign yield to dethTreasury
        uint88 dethTreasuryReward = yield.mul(dethTreasury).divU88(dethTotal);
        yield -= dethTreasuryReward;
        // Assign tithe of the remaining yield to treasuryF
        uint88 tithe = yield.mulU88(vault.dethTithePercent());
        yield -= tithe;
        // Calculate change in yield rate
        uint80 dethYieldRate = yield.divU80(dethCollateral);
        if (dethYieldRate == 0) return;
        // Realize new totals if yield rate increases after rounding
        TAPP.ethEscrowed += dethTreasuryReward + tithe;
        Vault.dethTotal = dethTotalNew;
        Vault.dethYieldRate += dethYieldRate;
        Vault.dethCollateralReward += yield;
    }

Assuming updateYield gets called, if there is no generated yield from dethTotalNew then a return will be done at if (dethTotalNew <= dethTotal) return; since dethTotalNew will equal dethTotal, which makes sense.
Lets assume there a generated yield and the code continue to execute until it gets at dethYieldRate = yield.divU80(dethCollateral);. Now we have dethYieldRate value, since we have yield generated and existing dethCollateral amount comes from bid and short orders. so what's the case?

The Case
if dethYieldRate value resulted as zero and a return after, then we could have an issue. Remember we said we have already generated yield? yes that's the issue there, this amount of generated yield should be updated into Vault.dethTotal all the time whenever we have new yield. but since we had a return at if (dethYieldRate == 0) return no update is being done to Vault.dethTotal.

this case also apply the same scenario on TAPP.ethEscrowed, clearly not being updated but only after the return which is unreachable.

is that possible that dethYieldRate == zero and we still have yield?
Yes :) please check POC.

The last impact which is important as well is _ethConversion method, _ethConversion is used in withdraw method, it's responsible to return the right amount for the withdrawal. However the impact is when s.vault[vault].dethTotal is used in the comparison if (dethTotalNew >= dethTotal) to determine the amount that will be returned to withdrawal. If Vault.dethTotal is not correctly up to date in updateYield then we will have an issue in the returned amount since it is possible that dethTotalNew > dethTotal if dethTotal didn't get updated in last updateYield, and then the withdrawal will get the whole amount instead of applying the right accounting for it amount.mul(dethTotalNew).divU88(dethTotal).

POC CODE: To run the poc code please add the code in a file with updateYieldIssue.t.sol under test\updateYieldIssue.t.sol

// SPDX-License-Identifier: GPL-3.0-only
pragma solidity 0.8.21;

import {U256, U88, U80} from "contracts/libraries/PRBMathHelper.sol";

import {Errors} from "contracts/libraries/Errors.sol";
import {C, VAULT} from "contracts/libraries/Constants.sol";
import {STypes, MTypes, O} from "contracts/libraries/DataTypes.sol";

import {OBFixture} from "test/utils/OBFixture.sol";
import {console} from "contracts/libraries/console.sol";

contract YieldTest is OBFixture {
    using U256 for uint256;
    using U88 for uint88;
    using U80 for uint80;

    function setUp() public virtual override {
        super.setUp();
    }

    function test_0_updateYield_Incorrect_yield_issue() public{
        //According to DeployHelpet.sol
        // a.minAskEth = 10; // 1 -> 0.1 ether
        // a.minShortErc = 2000; // 2000 -> 2000 ether
        vm.startPrank(owner);
        diamond.setMinAskEth(asset, 10); //10 deth
        diamond.setMinShortErc(asset, 2000); //2000 dusd
        vm.stopPrank(); 

        console.log("vault");
        console.log(vault);

        //deposit into bridge so  can't be zero
        address userA = vm.addr(123);
        deal(userA, 100000000 ether);

        uint80 price = 10 ether; //deth
        uint88 amount = 2000 ether; //dusd
        //generate 100 orders to incresae deth coleteral
        //instead of using many users we use only one
        for (uint160 i; i < 100; i++) {
            fundLimitShort(price,  amount, userA);
            fundLimitBid(price, amount, userA); 
        }

        // Grab state before
        uint256 getDethTotal = diamond.getDethTotal(vault);
        uint256 vault_dethTotal = diamond.getVaultStruct(vault).dethTotal;
        uint256 dethYieldRate = diamond.getVaultStruct(vault).dethYieldRate;
        uint256 yield = getDethTotal - vault_dethTotal;
        console.log("\n no update yield");
        console.log(yield);

        console.log("\n vault_dethTotal");
        console.log(vault_dethTotal);

        console.log("\n dethYieldRate");
        console.log(dethYieldRate);

        //update yield
        console.log("\n\n update yield is done");
        diamond.updateYield(vault);

        //Assert no change
        getDethTotal = diamond.getDethTotal(vault);
        vault_dethTotal = diamond.getVaultStruct(vault).dethTotal;
        dethYieldRate = diamond.getVaultStruct(vault).dethYieldRate;
        yield = getDethTotal - vault_dethTotal;

        console.log("\n yield");
        console.log(yield);

        console.log("\n vault_dethTotal");
        console.log(vault_dethTotal);

        console.log("\n dethYieldRate");
        console.log(dethYieldRate);

        console.log("\n getDethTotal");
        console.log(getDethTotal);

        //generate yield
        uint256 generate_amount = 200000; //0,0000000000002 ether
        uint256 startingAmt = bridgeSteth.getDethValue();
        uint256 endingAmt = startingAmt + generate_amount;
        deal(_steth, _bridgeSteth, endingAmt);
        console.log("\n\n yield generated ");
        console.log(generate_amount);

        //update yield
        console.log("\n\n update yield is done");
        diamond.updateYield(vault);

        //Assert no change
        getDethTotal = diamond.getDethTotal(vault);
        vault_dethTotal = diamond.getVaultStruct(vault).dethTotal;
        dethYieldRate = diamond.getVaultStruct(vault).dethYieldRate;
        yield = getDethTotal - vault_dethTotal;

        console.log("\n yield");
        console.log(yield);

        console.log("\n vault_dethTotal");
        console.log(vault_dethTotal);

        console.log("\n dethYieldRate");
        console.log(dethYieldRate);

        console.log("\n getDethTotal");
        console.log(getDethTotal);
        console.log("\n");

        //update yield
        console.log("\n update yield is done");
        diamond.updateYield(vault);

        getDethTotal = diamond.getDethTotal(vault);
        vault_dethTotal = diamond.getVaultStruct(vault).dethTotal;
        dethYieldRate = diamond.getVaultStruct(vault).dethYieldRate;
        yield = getDethTotal - vault_dethTotal;

        console.log("\n incorrect value of yield <<<<< must be zero");
        console.log(yield); //yield here must be zero

        console.log("\n not updated vault_dethTotal <<<<<<");
        console.log(vault_dethTotal); //is not up to date

        console.log("\n dethYieldRate");
        console.log(dethYieldRate);

        console.log("\n getDethTotal");
        console.log(getDethTotal);
        console.log("\n");
    }

}

//OUTPUT

Logs:
  vault
  1

 no update yield
  0

 vault_dethTotal
  12000000000000000000000000

 dethYieldRate
  0

 update yield is done

 yield
  0

 vault_dethTotal
  12000000000000000000000000

 dethYieldRate
  0

 getDethTotal
  12000000000000000000000000

 yield generated
  200000

 update yield is done

 yield
  200000

 vault_dethTotal
  12000000000000000000000000

 dethYieldRate
  0

 getDethTotal
  12000000000000000000200000

 update yield is done

 incorrect value of yield <<<<< must be zero
  200000

 not updated vault_dethTotal <<<<<<
  12000000000000000000000000

 dethYieldRate
  0

 getDethTotal
  12000000000000000000200000

Tools Used

Manual Review

Recommended Mitigation Steps

Always update TAPP.ethEscrowed and Vault.dethTotal in updateYield method.

Assessed type

Other

[c4-pre-sort]

raymondfam marked the issue as sufficient quality report

[c4-pre-sort]

raymondfam marked the issue as duplicate of #57

[raymondfam]

See #57.

[c4-judge]

hansfriese marked the issue as unsatisfactory: Invalid

[evokid]

It seems duplicated but actually it's not, The issue #57 has different root cause and different impact, the root cause of #57 issue that if (dethTotalNew <= dethTotal) return; while mine #242 is different which is if (dethYieldRate == 0) return , The warden at #57 issue is focusing on and describing the values that are related to dethYieldRate and dethCollateralRate, in my case I am mentioning specifically the inconsistence of updateYield that will affect _ethConversion in the withdraw as impact.

_ethConversion would return wrong amount in withdraw

which will be unfair for other users, since in _ethConversion it will return the same amount instead of jumping to the else block applying the correct accounting. In other words, the loss (negative yield) won't be proportionally incurred by all users.

and then the withdrawal will get the whole amount instead of applying the right accounting for it amount.mul(dethTotalNew).divU88(dethTotal).

on the other hand the warder's issue #57 is presenting a future impact:

future bridges which do not use bridge credit system would allow deth to be exchanged where 1 deth = 1 eth if system returns to yeild in zero or positive state

My issue is presenting an impact which is not related to the protocol design as the sponsor explained in #57 issue. In fact, my issue #242 has a practical impact on withdrawals and can be prevented (not necessarily is important to the sponsors). I appreciate the judge reviewing this issue again, this is my opinion anyway.

[ditto-eth]

Vault.dethTotal is not set until the very end precisely to avoid yield being lost. For example, if a user (or an attacker) called updateYield repeatedly when only small amounts of yield had been generated, the resulting zethYieldRate could round down to 0 but Vault.dethTotal would slowly creep up and users would never receive yield.

The proposed mitigation here would actually cause the problems mentioned

[c4-sponsor]

ditto-eth (sponsor) disputed

[hansfriese]

@evokid I want to hear your reply.

[evokid]

@ditto-eth Thank you for your feedback and explaining the case. I totally agree with the sponsor, after digging deep in the issue I found my mitigation is not strong, and will cause other problems. However, the issue still stand, and I would suggest a different mitigation:

• Create a state variable Vault.uptodateDethTotal to store dethTotalNew value always in updateYield method, even if the dethYieldRate is zero.
• On _ethConversion use Vault.uptodateDethTotal instead of Vault.dethTotal, it will apply in withdraw and withdrawTapp methods.

I believe this mitigation fix the issue without exposing the protocol to the mentioned risks by the sponsor. much appreciation for the judge @hansfriese.

[ditto-eth]

@evokid Can you provide an example with ethConversion()? vault.dethTotal can only be set during positive yield accrual, otherwise when users attempt to withdraw() then they will take away extra yield that hasn't been realized by the system. In the rare case of negative yield it seems safer to default to lower withdrawal amounts while the system recovers

[evokid]

@ditto-eth Thank you for feedback. Sure, we should assume that Vault.uptodateDethTotal should act always as Vault.dethTotal in the protocol, except it will be always updated in updateYield method. Now In the rare case of negative yield:

• Vault.dethTotal = x and Vault.uptodateDethTotal = y.
• The incorrect amount is amount.mul(dethTotalNew).divU88(x)
• The correct amount is amount.mul(dethTotalNew).divU88(y)
• Fair amount here should rely on y not x since y > x.
• As a final result the user's correct amount will be less than the incorrect amount which will be fair to other users.

This is safer and goes with your measure to default to lower amounts on withdrawal.

[hansfriese]

I agree with the sponsor. Furthermore, I've noticed LibVault.sol is out of scope.

[c4-judge]

hansfriese marked the issue as not a duplicate

[c4-judge]

hansfriese marked the issue as unsatisfactory: Out of scope

[evokid]

Hi @hansfriese, contracts/facets/BridgeRouterFacet.sol is in scope which include deposit() that calls maybeUpdateYield, also BridgeRouterFacet.sol contains _ethConversion as well, The sponsor asked for an example and I provided it as well, for the mitigation that he is looking for. thank you :)

[hansfriese]

The sponsor will mitigate if it's valid. According to the contest info, LibVault.sol should be treated as out of scope although it's used by other in scope contracts.

[evokid]

I respect your decision, thank you for your time and looking into it.