Deposit could be blocked when updateYield gets called in maybeUpdateYield

[c4-bot-1]

Lines of code

https://github.com/code-423n4/2024-03-dittoeth/blob/91faf46078bb6fe8ce9f55bcb717e5d2d302d22e/contracts/facets/BridgeRouterFacet.sol#L63

Vulnerability details

Impact

Users will not be able to deposit in the protocol.

Proof of Concept

from updateYield method:

    // Assign tithe of the remaining yield to treasuryF
    uint88 tithe = yield.mulU88(vault.dethTithePercent());
    yield -= tithe;

A revert could occur here yield -= tithe since tithe possibly could be greater than the yield.

How could tithe could possibly greater than the yield? In the protocol when a short or a bid having a match they will eligible to have a discount in matchIncomingBid and matchIncomingSell, however if a discount is active this will increase Vault.dethTitheMod with the time (depends on coming value according to its math).

I created POC foundry to apply this case practically in the protocol, e.g. if the generated yield is 2000000, and the Vault.dethTitheMod reach 10000 and dethTithePercent is 10 then result is, the tithe is 2002000, which is greater than the yield value which cause a revert.

This edge case impact will lead to a revert and blocking updating the yield, causing the deposits to be blocked too since maybeUpdateYield is calling updateYield in case there is already more than BRIDGE_YIELD_UPDATE_THRESHOLD (1000 ether) deposited.

To run the POC code please add the code in a file with deposit_revert_issue .t.sol under test\deposit_revert_issue.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_02_tithe() public{
        uint256 dethTithePercent = getTithe(vault); //====> 1001000000000000000
        console.log("dethTithePercent");
        console.log(dethTithePercent);

        uint88 yield = 2000000; //generated yield
        console.log("yield");
        console.log(yield);
        //dethTithePercent method  ==  getTithe method in the protocol
        uint88 tithe = yield.mulU88(dethTithePercent);
        console.log("tithe");
        console.log(tithe);

        //as a result tithe become less than yield in updateYield method
        //causing a revert and blocking updating the yield, causing the 
        //deposits to be blocked.
    }

    //simulate same math of getTithe in the protocol
     function getTithe(uint256 vault) public view returns (uint256) {
        uint256 dethTithePercent = 10; //set initally by the DAO
        uint16 dethTitheMod = 10000; //increase and decreas according to orders discounts
        return (uint256(dethTitheMod + dethTithePercent) * 1 ether) / C.FOUR_DECIMAL_PLACES;
    }

}

Output

Logs:
  dethTithePercent
  901000000000000000
  yield
  2000000
  tithe
  1802000

Tools Used

Manual Review

Recommended Mitigation Steps

check if tithe > yield then return without reverting.

Assessed type

Other

[c4-pre-sort]

raymondfam marked the issue as sufficient quality report

[c4-pre-sort]

raymondfam marked the issue as primary issue

[raymondfam]

Will let sponsor review the coded POC.

[c4-sponsor]

ditto-eth (sponsor) disputed

[ditto-eth]

the poc is invalid, but having dived a little deeper this can be a configuration issue for the dao if tithe set to specific values, which is not the case for our config.

[hansfriese]

As it's related to the admin's configuration, QA is appropriate.

[c4-judge]

hansfriese changed the severity to QA (Quality Assurance)

[c4-judge]

hansfriese marked the issue as grade-c