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
Rank: 3/5
Findings: 1
Award: $0.00
🌟 Selected for report: 2
🚀 Solo Findings: 1
🌟 Selected for report: T1MOH
Data not available
https://github.com/code-423n4/2023-11-betafinance/blob/main/Omni_Protocol/src/OmniToken.sol#L76-L143
Protocol mints incorrect depositAmount and depositShare to protocol. Such that reserveFee is higher than defined. Suppose following scenario:
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.
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.
5_000 * 20% = 1_000
. For Tranche1 is 10_000 * 10% = 1_000
1000 * 20_000 / 40_000 = 500
, tranche2 receives 500 too. Finally tranche2 has 1500 of interest, tranche1 has 500 of interest1500 * 10% = 150
in tranche2, and 500 * 10% = 50
in tranche1totalDepositAmount
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.
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 Tranche11) (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
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
Data not available
https://github.com/code-423n4/2023-11-betafinance/blob/main/Omni_Protocol/src/OmniOracle.sol#L48
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
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
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); }
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))); }
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