Beta Finance Invitational - ladboy233's results

Judge has assessed an item in Issue #19 as 2 risk. The relevant finding follows:

Borrower can abuse enterMarkets to force liquidator can pay more fund

Lines of code

https://github.com/code-423n4/2023-11-betafinance/blob/0f1bb077afe8e8e03093c8f26dc0b7a2983c3e47/Omni_Protocol/src/OmniToken.sol#L76 https://github.com/code-423n4/2023-11-betafinance/blob/0f1bb077afe8e8e03093c8f26dc0b7a2983c3e47/Omni_Protocol/src/OmniPool.sol#L303 https://github.com/code-423n4/2023-11-betafinance/blob/0f1bb077afe8e8e03093c8f26dc0b7a2983c3e47/Omni_Protocol/src/OmniToken.sol#L152 https://github.com/code-423n4/2023-11-betafinance/blob/0f1bb077afe8e8e03093c8f26dc0b7a2983c3e47/Omni_Protocol/src/OmniToken.sol#L180

Vulnerability details

Impact

MEV bot can frontrun user's repayment to liquidate user first when the OmniPool is unpaused

Proof of Concept

this report tries to combine a few issue

1. when OmniPool is paused, interest is still accuring
2. when OmniPool is paused, user cannot repay
3. user cannot repay to make the account health, use can deposit more asset to make account health, but user can deposit but cannot withdraw, meaning user's deposit fund is locked

note that the repay function has WhenNotPaused modifier

if the OmniPool is paused for a long period of time,

then a malicious liquidator can keep calling accure to force user pay the interest

and when when admin unpause the contract, there is no time lock for user to repay the debt to reduce the borrow account, and the user's is forced to enter the unhealthy state

liquidator cannot liquidate user when the contract is paused, but they can monitor the unpause transaction

MEV bot can frontrun user's repayment to liquidate user first when OmniPool is unpaused

or it is possible during the pause period, the collateral asset price drops and when contract unpaused, before user repay, MEV bot liquidate and seize user asset first to capture liquidation reward

Tools Used

Manual Review

Recommended Mitigation Steps

enable repayment even when contract is paused, applies a timelock window for user to repay the debt when contract is unpaused,

do not charge interest when contract is paused

Assessed type

Invalid Validation

Lines of code

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

Vulnerability details

Impact

interest is still accuring when the market is paused, force user to incur debts

Proof of Concept

when the function accure is called

the interest is accured after the interest rate is calculated

{
uint256 interestRate = IIRM(irm).getInterestRate(address(this), trancheIndex, totalDeposit, totalBorrow);
interestAmount = (trancheBorrowAmount_ * interestRate * timePassed) / 365 days / IRM_SCALE;
}

the interest rate depends on the time passed as well

the longer time passed, the higher the interest paid

however, even when the market / tranche is paused

the user interest is still accuring

but repay is not a option because the repay function is guarded by whenNotPaused modifier

    function repay(uint96 _subId, address _market, uint256 _amount) external whenNotPaused {

then user cannot make account health and write off debt by repaying

the only option that user has is to deposit more fund to make the account healthy

but when the contract is paused, user can deposit but cannot really withdraw

    function withdraw(uint96 _subId, uint8 _trancheId, uint256 _share) external nonReentrant returns (uint256 amount) {

the withdraw function is guarded by nonReentrant as well

so any fund that user use to make the account health is locked until the contract is paused

Tools Used

Manual

Recommended Mitigation Steps

do not incur interest when the tranche is paused

Assessed type

Timing

Lines of code

https://github.com/code-423n4/2023-11-betafinance/blob/0f1bb077afe8e8e03093c8f26dc0b7a2983c3e47/Omni_Protocol/src/OmniPool.sol#L348 https://github.com/code-423n4/2023-11-betafinance/blob/0f1bb077afe8e8e03093c8f26dc0b7a2983c3e47/Omni_Protocol/src/OmniToken.sol#L180

Vulnerability details

Impact

paucheTranche state can be set to arbitrary value

Proof of Concept

the protocol has this concept of tranche id and borrower tier, the higher borrower tier means high risk

lower borrower tier means low risk

but when liquidation happens

if the borrowTrueValue is greater than depositTrueValue, which can happen because of the underlying oracle price can change

if (evalAfter.borrowTrueValue > evalAfter.depositTrueValue) {
	pauseTranche = borrowTier;
	emit PausedTranche(borrowTier);
}

when the pauseTranche id is set to the borrower tier in this line of code

then the user that deposit below the paused tranche id should not be able to withdraw / tranche id share

because this check is in-place

require(_trancheId < IOmniPool(omniPool).pauseTranche(), "OmniToken::withdraw: Tranche paused.");

but the problem is that the paucheTranche state can be set to arbitrary value

suppose a user has borrower tier 10

and borrowTrueValue exceed depositTrueValue, the pauseTranche is set to 10

then a second user has borrower tier 0

he is subject to liquidation as well

and borrowTrueValue exceed depositTrueValue as well

the pauseTranche is reset to 0 again, which defeat the check of

require(_trancheId < IOmniPool(omniPool).pauseTranche(), "OmniToken::withdraw: Tranche paused.");

Tools Used

Manual Review

Recommended Mitigation Steps

change the code to

if (evalAfter.borrowTrueValue > evalAfter.depositTrueValue) {
	pauseTranche = max(borrowTier, pauseTranche)
	emit PausedTranche(borrowTier);
}

Assessed type

Invalid Validation

require(_trancheId < IOmniPool(omniPool).pauseTranche(), "OmniToken::withdraw: Tranche paused.");

if (evalAfter.borrowTrueValue > evalAfter.depositTrueValue) {
	pauseTranche = min(borrowTier, pauseTranche)
	emit PausedTranche(borrowTier);
}

Lines of code

https://github.com/code-423n4/2023-11-betafinance/blob/0f1bb077afe8e8e03093c8f26dc0b7a2983c3e47/Omni_Protocol/src/OmniToken.sol#L104

Vulnerability details

Impact

loss of precision is too high when accuring interest

Proof of Concept

When intereste accures, we are calling

uint256 interestAmount;
{
	uint256 interestRate = IIRM(irm).getInterestRate(address(this), trancheIndex, totalDeposit, totalBorrow);
	interestAmount = (trancheBorrowAmount_ * interestRate * timePassed) / 365 days / IRM_SCALE;
}

note that the loss of precision is too high:

	interestAmount = (trancheBorrowAmount_ * interestRate * timePassed) / 365 days / IRM_SCALE;

we are dividing 365 days and then divde by IRM_SCALE (which is 1e9)

combing the fact taht the IRM_SCALE is hardcoded to 1e9

and if the time passed between two accuring is small,

and the underlying token have low decimals (6 decimals, even two decimals)

the interest accured will be always rounded to 0

In tests/MockERC20.sol,

the decimal is hardcoded to 18

we change the hardcode decimal to 6

and we add the POC below in TestOmniToken.t.sol

    function test_Accrue_POC_Low_decimal() public {
        setUpBorrow();
        (uint256 tda0, uint256 tba0,,) = oToken.tranches(0);
        (uint256 tda1, uint256 tba1,,) = oToken.tranches(1);
        (uint256 tda2, uint256 tba2,,) = oToken.tranches(2);
        uint256 td = tda0 + tda1 + tda2;

        uint256 borrowAmount = 10 * (10 ** uToken.decimals());
        IOmniPool(pool).borrow(0, address(oToken), borrowAmount);

        uint256 time_elapse = 120 seconds;
        vm.warp(time_elapse);

        uint256 interestAmount;
        {
            uint256 interestRate = irm.getInterestRate(address(oToken), 0, td, borrowAmount);
            interestAmount = borrowAmount * interestRate * time_elapse / 365 days / oToken.IRM_SCALE();
        }
        uint256 feeInterestAmount = interestAmount * oToken.RESERVE_FEE() / oToken.FEE_SCALE();
        interestAmount -= feeInterestAmount;

        oToken.accrue();

        vm.warp(time_elapse * 2);

        oToken.accrue();

        vm.warp(time_elapse * 4);

        oToken.accrue();
    
    }

we are accuring the interest every two minutes

before running the POC

we import the

import "forge-std/console.sol";

In OmniToken.sol

and add the console.log to log the interest

{
	uint256 interestRate = IIRM(irm).getInterestRate(address(this), trancheIndex, totalDeposit, totalBorrow);
	// @audit
	// this can be trancated to 0
	interestAmount = (trancheBorrowAmount_ * interestRate * timePassed) / 365 days / IRM_SCALE;
	console.log("interestAmount", interestAmount);
}

then we run the POC

forge test -vv --match-test "test_Accrue_POC_Low_decimal"

the output is

Running 1 test for src/tests/TestOmniToken.t.sol:TestOmniToken
[PASS] test_Accrue_POC_Low_decimal() (gas: 737833)
Logs:
  interestAmount 0
  interestAmount 0
  interestAmount 0

this mean within every two minutes time elapse, the interest is round down to 0

and the accure function is called in every borrow / withdraw / transfer, so the interest will be rounded down heaviliy

or user can keep calling accure for every two minutes (or x minutes) to avoid paying interest

Tools Used

Manual Review

Recommended Mitigation Steps

modify the interest, do not do muliplication before division to avoid precision loss

do not hardcode IRM_SCALE and consider token decimals when accuring interess

Assessed type

Decimal

Should validate if duplicate market exists when admin set mode

when admin set the mode

   function setModeConfiguration(ModeConfiguration memory _modeConfiguration)
        external
        onlyRole(MARKET_CONFIGURATOR_ROLE)
    {
        if (_modeConfiguration.expirationTimestamp <= block.timestamp) { revert("OmniPool::setModeConfiguration: Bad expiration timestamp."); }
        modeCount++;
        modeConfigurations[modeCount] = _modeConfiguration;
        emit SetModeConfiguration(modeCount, _modeConfiguration);
    }

the code should validate if duplicate markets exists in the mode configuration to avoid double evaluting and counting user collateral

In OmniTokenNoBorrow.sol and OmniToken.sol, only first 25 account counts toward the user balance

balanceOf in OmniToken only iteravte over first 25 account to count the user balance

but user can create sub account in uint96 space

then when external api replies on the balanceOf to query the user balance, the returned result undercounts under balance if the user's sub account number exceed 25

if the protocol has the need to expose the user balance

the protocol should update the user balance when user deposit the protocol should reduce the user balance when user withdraw or transfer

and then return the user balance in O(1) time instead of in O(n) time when running the for loop

OmniTokenNoBorrow.sol and OmniToken.sol is not capable of handling airdrop token

OmniTokenNoBorrow.sol and OmniToken.sol is likely to hold a large amount of asset

in case when there is a project issue airdrop based on the token balance,

the token pool is not capable of claim airdrop and cannot transfer the airdrop out from the token pool

then the airdrop is lost

In OmniPool.sol, enterMarkets does not validate if borrow cap is reached

when enterMarket, the protocol only validate when the borrow cap is not 0

require(IOmniToken(market).getBorrowCap(0) > 0, "OmniPool::enterMarkets: Market has no borrow cap for 0 tranche.");

but does not validate the BorrowCap of the market is reachced

in case when BorrowCap of the market is reachced, user cannot really borrow but still paying the gas to call accure every time for that added market

Use a more recent version of openzeppelin

the protocol openzepplein used is 4.8.0, but the newly version is 4.9+

recommend using a more recent version of openzeppelin to avoid bugs

WETH address should not be hardcoded in multichain setting

the protocol wants to deploy the contract in both eth mainnet and BSC

but the WETH address is hardcoded to ETH address in mainnet

    address public constant WETH = 0xC02aaA39b223FE8D0A0e5C4F27eAD9083C756Cc2; // Need to hardcode WETH address per network deployed for Band

as the comment suggest,

the protocol needs to hardcode WETH address per network deployed for Band

should perform additional validation for oracle when Provider.Other is used

in this line of code

} else if (config.provider == Provider.Other) {
	return IOmniOracle(config.oracleAddress).getPrice(_underlying) * (PRICE_SCALE / 1e18) / (10 ** IERC20Metadata(_underlying).decimals());
} 

in case when token oracle is not provided by band or chainlink, Provider.Other is used

if the provider.OTHER is used, the protocol should validate if the price is stale or return 0 as well instead of use the oracle price directly

because the oracle price deposit collateral value and incorrect value can result in loss of fund (false liquidation / overborrowing)

should not validate the token oracle by symbol

When band oracle is used, if the token is not WETH, the type of the oracle query is constructed based on the token symbol

data = IStdReference(config.oracleAddress).getReferenceData(IERC20Metadata(_underlying).symbol(), USD);

however, the underlying address returned symbol can change, so it is recommend to cache the token symbol and validate by token address instead of using the returned token symbol to construct query data

should cache the token decimals in oracle

when oracle is used, every time the token decimal is fetched when return the oracle price

return uint256(answer) * (PRICE_SCALE / (10 ** IChainlinkAggregator(config.oracleAddress).decimals())) / (10 ** IERC20Metadata(_underlying).decimals());

but the underlying token returned decimal can change

so it is recommend the cache the decimal for token

liquidation / borrow / withdraw revert and blocked in the oracle price becomes stale or price feed is deprecated

chainlink reguarly deprecate the feed

https://docs.chain.link/data-feeds/deprecating-feeds?network=deprecated&page=1

in case when the feed is deprecated,

calling IChainlinkAggregator(config.oracleAddress).latestRoundData() wilil result in revert

or when the price is stale, both chainlink oracle or band oracle revert

when it is correct to not use the incorrect or misreported price

the liquidation flow / borrow / withdraw flow that replies on the oracle price is blocked and revert until the feed is updated by admin or the price not become stale