Beta Finance Invitational - T1MOH's results

Personalized lending and borrowing with zero liquidity fragmentation and maximal capital efficiency on the EVM.

General Information

Platform: Code4rena

Start Date: 01/11/2023

Pot Size: $24,150 USDC

Total HM: 5

Participants: 5

Period: 5 days

Judge: cccz

Total Solo HM: 4

Id: 303

League: ETH

Beta Finance

Findings Distribution

Researcher Performance

Rank: 3/5

Findings: 1

Award: $0.00

QA:
grade-a

🌟 Selected for report: 2

🚀 Solo Findings: 1

Findings Information

🌟 Selected for report: T1MOH

Labels

bug
2 (Med Risk)
primary issue
satisfactory
selected for report
sponsor confirmed
M-04

Awards

Data not available

External Links

Lines of code

https://github.com/code-423n4/2023-11-betafinance/blob/main/Omni_Protocol/src/OmniToken.sol#L76-L143

Vulnerability details

Impact

Protocol mints incorrect depositAmount and depositShare to protocol. Such that reserveFee is higher than defined. Suppose following scenario:

  1. Tranche 2 has 20% APR, has 5_000 borrowed
  2. Tranche 1 has 10% APR, has 10_000 borrowed
  3. ReserveFee is 10%
  4. It means that reserveFee that must be payed after 1 year is 5_000 * 20% * 10% + 10_000 * 10% * 10% = 200. However current implementation will calculate 208, which will be shown PoC.

Lender pays this extra fee.

Proof of Concept

Here is gist with all tests https://gist.github.com/T1MOH593/34729b5333fe43eb58cf8b4948ef137f

First part: show that issue exists. It is shown in custom_test1()

Second part: what is the issue. Let's set initial values, for example: Tranche 2 has 20% APR, 5_000 borrowed, 20_000 deposited; Tranche 1 has 10% APR, 10_000 borrowed, 20_000 depositeed. And we use 1 year as time difference to ease calculations. Tranche struct has 2 types of variables, let's focus regarding deposit: totalDepositAmount and totalDepositShare. Initially without accrued interest they equal, 1 : 1, in above scenario will be 20_000 in both tranches. As the time passes, interest accrues. It means that totalDepositAmount increases. If reserveFee is 0%, that's it - for example 1000 of interests accrued, totalDepositAmount is increased by 1000.

However in case 10% of interest must be payed to reserve. It means that extra shares will be minted increasing totalDepositShare AND totalDepositAmount should be also increased because otherwise user's balance will decrease. Let's take delta of totalDepositAmount as X and totalDepositShare as Y. And calculate these values for above scenario.

  • Total interest for Tranche2 is 5_000 * 20% = 1_000. For Tranche1 is 10_000 * 10% = 1_000
  • Interest for Tranche 2 stays in this tranche. Interest for Tranche 1 is divided between tranche1 and tranche2 based on deposit amounts, such that tranche1 receives 1000 * 20_000 / 40_000 = 500, tranche2 receives 500 too. Finally tranche2 has 1500 of interest, tranche1 has 500 of interest
  • Reserve fee must be payed. 1500 * 10% = 150 in tranche2, and 500 * 10% = 50 in tranche1
  • We need to mint such amount of share Y to Reserve and increase totalDepositAmount by X so that Reserve balance is 150 AND User's balance is 20_000 + 1500 * 90% = 21350 in Tranche2. So let's create system of equations.
(20_000 / (20_000 + Y)) * (20_000 + X) = 21_350 is balance of User Y / (20_000 + Y) * (20_000 + X) = 150 is balance of Reserve

Solving this we get X = 1500, Y = 140.51. I.e. 140.51 of shares must be minted to Reserve, and totalDepositAmount must be increased by 1500 (amount of accrued interest) in Tranche2. Now let's calculate accordingly for Tranche1:

(20_000 / (20_000 + Y)) * (20_000 + X) = 20_450 is balance of User Y / (20_000 + Y) * (20_000 + X) = 50 is balance of Reserve

X = 500, Y = 48.89.

However current implementation mints 100 shares to Reserve and increases totalDepositAmount by 1450 in Tranche 2; Also mints 100 shares to reserve and increases totalDepositAmount by 550 in Tranche1 as shown in test custom_test2

That is core issue: code always mints amount of shares equal to reserveInterest to Reserve (100 in this example) and calculates sub-optimal amount of depositAmount to increase.

Tools Used

Foundry

Fix is not obvious at all, it's the most difficult part of report to be honest. Algorithm should be completely refactored, should be implemented following accounting:

  • totalDepositAmount should be increased by the amount of accrued interest allocated to this tranche. From above example it's 1500 for Tranche2 and 500 for Tranche1
  • Number of shares minted to reserve in tranche should be calculated as solution of one of these equations
1) (totalDepositSharesOfTranceBeforeAccruing / (totalDepositSharesOfTranceBeforeAccruing + Y)) * (totalDepositAmountBeforeAccruing + interestAmountAllocatedToThisTrance) = totalDepositSharesOfTranceBeforeAccruing + interestAmountAllocatedToThisTrance * 0.9 If we paste values from above example with Tranche 2, we get (20_000 / (20_000 + Y)) * (20_000 + 1_500) = 21_350 2) (Y / (totalDepositSharesOfTranceBeforeAccruing + Y)) * (totalDepositAmountBeforeAccruing + interestAmountAllocatedToThisTrance) = interestAmountAllocatedToThisTrance * 0.9 If we paste values from above example with Tranche 2, we get (Y / (20_000 + Y)) * (20_000 + 1_500) = 150

And also write tests to check reserveFee amounts, now they absent

Assessed type

Math

#0 - JeffCX

2023-11-07T01:41:38Z

Don't think this is a vulnerability

#1 - thereksfour

2023-11-07T15:17:24Z

I've noticed this issue before, the reason for it is that the reserveShare is calculated by dividing by trancheDepositAmount_ instead of trancheDepositAmount_+interestAmountProportion, so the calculated reserveShare will be larger.

                tranche.totalDepositAmount = trancheDepositAmount_ + interestAmountProportion + reserveInterestAmount;
                tranche.totalBorrowAmount = trancheBorrowAmount_ + depositInterestAmount + reserveInterestAmount;
            }

            // Pay reserve fee
            uint256 reserveShare;
            uint256 totalDepositShare_ = tranche.totalDepositShare;
            if (trancheDepositAmount_ == 0) {
                reserveShare = reserveInterestAmount;
            } else {
                reserveShare = (reserveInterestAmount * totalDepositShare_) / trancheDepositAmount_; // Cannot divide by 0
            }
            trancheAccountDepositShares[trancheIndex][reserveReceiver] += reserveShare;
            tranche.totalDepositShare = totalDepositShare_ + reserveShare;

A simple example is

Tranche 1 has 5% APR, has 10000 borrowed ReserveFee is 10%. interestAmount = 10000 * 5% = 500 reserveInterestAmount = 500 * 10% = 50 interestAmountProportion = 500 * 90% = 450 cur: reserveShare = 50 * 10000 / 10000 = 50 reserveAmount = 50 * (10000+500) / (1000+50) = 52.24 fix: reserveShare = 50 * 10000 / (10000+450) = 47.847 reserveAmount = 47.847 * (10000+500) / (10000+47.847) = 50

I previously thought this was intentional because it was to handle the case where interestAmountProportion wouldn't be distributed when trancheDepositAmount_ = 0. But thinking about it again, it's just a simple fix

            if (trancheDepositAmount_ == 0) {
-               reserveShare = reserveInterestAmount;
+               reserveShare = reserveInterestAmount + interestAmountProportion;
            } else {
-               reserveShare = (reserveInterestAmount * totalDepositShare_) / trancheDepositAmount_; // Cannot divide by 0
+               reserveShare = (reserveInterestAmount * totalDepositShare_) / trancheDepositAmount_ + interestAmountProportion; // Cannot divide by 0
            }

#2 - c4-judge

2023-11-07T15:17:36Z

thereksfour marked the issue as primary issue

#3 - c4-judge

2023-11-07T15:18:22Z

thereksfour marked the issue as satisfactory

#4 - JeffCX

2023-11-07T15:34:55Z

Emm not sure the severity is medium

in this example

cur: reserveShare = 50 * 10000 / 10000 = 50 reserveAmount = 50 * (10000+500) / (1000+50) = 52.24 fix: reserveShare = 50 * 10000 / (10000+450) = 47.847 reserveAmount = 47.847 * (10000+500) / (10000+47.847) = 50

basically without the fix 2.24 more share is minted, which is not that significant and it generate more income for protocol

#5 - thereksfour

2023-11-07T15:52:10Z

Hah, It's bad when the borrowed token is WBTC. And generating income for protocol means making users lose, satisfying medium risk.

#6 - allenjlee

2023-11-12T15:44:17Z

I believe this is a valid issue, we have resolved the issue and reserve payments are correct now. See screenshot from the PoC test case.

<img width="495" alt="Screenshot 2023-11-12 at 11 43 58 PM" src="https://github.com/code-423n4/2023-11-betafinance-findings/assets/24996586/3e29d415-1cdd-4dcc-b59a-fbbba9284dca">

#7 - c4-sponsor

2023-11-12T15:44:24Z

allenjlee (sponsor) confirmed

#8 - c4-judge

2023-11-13T05:21:25Z

thereksfour marked the issue as selected for report

Findings Information

🌟 Selected for report: T1MOH

Also found by: 0xStalin, bin2chen, dirk_y, ladboy233

Labels

bug
grade-a
QA (Quality Assurance)
selected for report
sponsor confirmed
Q-04

Awards

Data not available

External Links

1. Low. OmniOracle.sol doesn't work with tokens returning symbol as bytes32

https://github.com/code-423n4/2023-11-betafinance/blob/main/Omni_Protocol/src/OmniOracle.sol#L48

Vulnerability details

Some tokens (e.g. MKR) have metadata fields (name / symbol) encoded as bytes32 instead of the string prescribed by the ERC20 specification. Thus such tokens can't be used in Band Oracle

    function getPrice(address _underlying) external view returns (uint256) {
        OracleConfig memory config = oracleConfigs[_underlying];
        if (config.provider == Provider.Band) {
            IStdReference.ReferenceData memory data;
            if (_underlying == WETH) {
                data = IStdReference(config.oracleAddress).getReferenceData("ETH", USD);
            } else {
@>              data = IStdReference(config.oracleAddress).getReferenceData(IERC20Metadata(_underlying).symbol(), USD);
            }
            ...
    }

Use the BoringCrypto safeSymbol() function code with the returnDataToString() parsing function to handle the case of a bytes32 return value: https://github.com/boringcrypto/BoringSolidity/blob/ccb743d4c3363ca37491b87c6c9b24b1f5fa25dc/contracts/libraries/BoringERC20.sol#L15-L39

2. Low. Restrict updating isIsolatedCollateral from true to false and vice-versa

Vulnerability details

https://github.com/code-423n4/2023-11-betafinance/blob/main/Omni_Protocol/src/OmniPool.sol#L508-L522

Current implementation allows to set new config. In case normal market will be marked isolated or vice versa, it will break internal accounting of OmniPool.sol. Because code assumes that this variable will never change

Explicitly disallow updating isIsolatedCollateral on configured markets

3. Low. Remove dust deposit amount after socializing loss

Vulnerability details

https://github.com/code-423n4/2023-11-betafinance/blob/main/Omni_Protocol/src/OmniPool.sol#L397

There can be dust amount of deposit socializeLoss is called

    function socializeLoss(address _market, bytes32 _account) external onlyRole(DEFAULT_ADMIN_ROLE) {
        uint8 borrowTier = getAccountBorrowTier(accountInfos[_account]);
        Evaluation memory eval = evaluateAccount(_account);
        uint256 percentDiff = eval.depositTrueValue * 1e18 / eval.borrowTrueValue;
@>      require(percentDiff < 0.00001e18, "OmniPool::socializeLoss: Account not fully liquidated, please call liquidate prior to fully liquidate account.");
        IOmniToken(_market).socializeLoss(_account, borrowTier);
        emit SocializedLoss(_market, borrowTier, _account);
    }

However it socializes all debt of user and leaves deposit to the user. But it should firstly reduce user's debt by that deposit, and then to socialize debt.

    function socializeLoss(bytes32 _account, uint8 _trancheId) external nonReentrant {
        require(msg.sender == omniPool, "OmniToken::socializeLoss: Bad caller");
        uint256 totalDeposits = 0;
        for (uint8 i = _trancheId; i < trancheCount; ++i) {
            totalDeposits += tranches[i].totalDepositAmount;
        }
        OmniTokenTranche storage tranche = tranches[_trancheId];
        uint256 share = trancheAccountBorrowShares[_trancheId][_account];
@>      //@audit It is debt amount which must be socialized
        uint256 amount = Math.ceilDiv(share * tranche.totalBorrowAmount, tranche.totalBorrowShare); // Represents amount of bad debt there still is (need to ensure user's account is emptied of collateral before this is called)
        uint256 leftoverAmount = amount;
@>      //@audit full amount is socialized
        for (uint8 ti = trancheCount - 1; ti > _trancheId; --ti) {
            OmniTokenTranche storage upperTranche = tranches[ti];
            uint256 amountProp = (amount * upperTranche.totalDepositAmount) / totalDeposits;
            upperTranche.totalDepositAmount -= amountProp;
            leftoverAmount -= amountProp;
        }
        tranche.totalDepositAmount -= leftoverAmount;
        tranche.totalBorrowAmount -= amount;
        tranche.totalBorrowShare -= share;
        trancheAccountBorrowShares[_trancheId][_account] = 0;
        emit SocializedLoss(_account, _trancheId, amount, share);
    }

Subtract user's deposit before socializing debt

    function socializeLoss(bytes32 _account, uint8 _trancheId) external nonReentrant {
        require(msg.sender == omniPool, "OmniToken::socializeLoss: Bad caller");
        uint256 totalDeposits = 0;
+       uint256 userDepositShares = 0;
        for (uint8 i = _trancheId; i < trancheCount; ++i) {
            totalDeposits += tranches[i].totalDepositAmount;
+           userDepositShares += trancheAccountDepositShares[i][_account];
        }
        OmniTokenTranche storage tranche = tranches[_trancheId];
        uint256 share = trancheAccountBorrowShares[_trancheId][_account];
        uint256 amount = Math.ceilDiv(share * tranche.totalBorrowAmount, tranche.totalBorrowShare); // Represents amount of bad debt there still is (need to ensure user's account is emptied of collateral before this is called)
+       
+       amount -= userDepositShares * tranche.totalDepositAmount / tranche.totalDepositShare;
+       
        uint256 leftoverAmount = amount;
        for (uint8 ti = trancheCount - 1; ti > _trancheId; --ti) {
            OmniTokenTranche storage upperTranche = tranches[ti];
            uint256 amountProp = (amount * upperTranche.totalDepositAmount) / totalDeposits;
            upperTranche.totalDepositAmount -= amountProp;
            leftoverAmount -= amountProp;
        }
        tranche.totalDepositAmount -= leftoverAmount;
        tranche.totalBorrowAmount -= amount;
        tranche.totalBorrowShare -= share;
        trancheAccountBorrowShares[_trancheId][_account] = 0;
        emit SocializedLoss(_account, _trancheId, amount, share);
    }

4. R. toAddress() logic can be simpler without ADDRESS_MASK

https://github.com/code-423n4/2023-11-betafinance/blob/main/Omni_Protocol/src/SubAccount.sol#L27

    function toAddress(bytes32 _account) internal pure returns (address) {
-       return address(uint160(uint256(_account) & ADDRESS_MASK));
+       return address(uint160(uint256(_account)));
    }

5. R. Remove unused variable from struct ModeConfiguration

Field modeMarketCount is never used, consider removing it

#0 - c4-judge

2023-11-07T17:18:48Z

thereksfour marked the issue as grade-a

#1 - allenjlee

2023-11-11T05:47:32Z

Good quality QA report. All good points. For 1., I think to handle issues w/ symbol better we should create an explicit mapping in our protocol, e.g. mapping(address => string) public underlyingSymbols. This should handle these issues as well as other potential issues w/ token symbol changes, etc.

For 2. added:

MarketConfiguration memory currentConfig = marketConfigurations[_market]; if (currentConfig.borrowFactor != 0 || currentConfig.collateralFactor != 0) { require( _marketConfig.isIsolatedCollateral == currentConfig.isIsolatedCollateral, "OmniPool::setMarketConfiguration: Cannot change isolated collateral status." ); }

For 3. think this is a fair point, tried to be practical here and accept some rounding issues when liquidating s.t. there's a $1000 left on $10 million debt value (hopefully there's never $10 million bad debt). I think the mitigation should be slightly adjusted though, as the userDepositShare could result in a different amount depending on each tranche. So would need to sum the amounts across each tranche then subtract that amount.

For 4., good point.

For 5., also good point.

#2 - c4-sponsor

2023-11-11T05:47:36Z

allenjlee (sponsor) confirmed

#3 - c4-judge

2023-11-15T05:24:36Z

thereksfour marked the issue as selected for report

#4 - thereksfour

2023-11-15T05:27:47Z

All entries in this QA report are valid, and plus the downgraded issues, this QA report was selected as the best

AuditHub

A portfolio for auditors, a security profile for protocols, a hub for web3 security.

Built bymalatrax © 2024

Auditors

Browse

Contests

Browse

Get in touch

ContactTwitter