INIT Capital Invitational - ladboy233's results

Lines of code

https://github.com/code-423n4/2023-12-initcapital/blob/a53e401529451b208095b3af11862984d0b32177/contracts/core/InitCore.sol#L535 https://github.com/code-423n4/2023-12-initcapital/blob/a53e401529451b208095b3af11862984d0b32177/contracts/lending_pool/LendingPool.sol#L161

Vulnerability details

Impact

Interest still accuring when repayment is paused

Proof of Concept

When the admin pause the lending pool repayment,

as timestamp elapses,

interest still accuring

  /// @inheritdoc ILendingPool
    function accrueInterest() public {
        uint _lastAccruedTime = lastAccruedTime;
        if (block.timestamp != _lastAccruedTime) {
            uint _totalDebt = totalDebt;
            uint _cash = cash;
            uint borrowRate_e18 = IIRM(irm).getBorrowRate_e18(_cash, _totalDebt);
            uint accruedInterest = (borrowRate_e18 * (block.timestamp - _lastAccruedTime) * _totalDebt) / ONE_E18;
            uint reserve = (accruedInterest * reserveFactor_e18) / ONE_E18;
            if (reserve > 0) {
                _mint(treasury, _toShares(reserve, _cash + _totalDebt + accruedInterest - reserve, totalSupply()));
            }
            totalDebt = _totalDebt + accruedInterest;
            lastAccruedTime = block.timestamp;
        }
    }

Interest accruing only depends on the time elapsed, but not whether the repayment is paused. This can create debt for user, and make user account unhealthy and eventually user’s position is subject to liquidation.

even when admin unpause the repayment, MEV bot can frontrun user's repayment and liqudiate user.

Tools Used

Manual Review

Recommended Mitigation Steps

Consider not accruing interest when repayment is paused, or not allowing to disable repayment.

Assessed type

Timing

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

Remove WLP from whitelist should not block user from removing WLP

Lines of code

https://github.com/code-423n4/2023-12-initcapital/blob/a53e401529451b208095b3af11862984d0b32177/contracts/core/PosManager.sol#L235

Vulnerability details

Impact

Lack of way to handle not fully repaid bad debt after liquidation after the lending pool share or WLP are fully seized

Proof of Concept

When user has bad debt, user's borrow credit > collateral credit

liqudiator can steps in liquidate and seize user's share or seize user WLP and repay the debt

while the liquidate function aims to let liqudiator take the min share available for bad debt repayment

// take min of what's available (for bad debt repayment)
shares = shares.min(IPosManager(POS_MANAGER).getCollAmt(_posId, _poolOut)); // take min of what's available
_require(shares >= _minShares, Errors.SLIPPAGE_CONTROL);

after the user's pool share is transferred out, or after user's WLP is seized, the rest of unpaid debt becomes bad debt permanently

for example,

user's borrow 1000 USDT and has debt 1000 USDT, his share only worth 500 USD as collateral price drops

because the code let liquidator takes min of what's available for both lending pool share and take min of what's available (for bad debt repayment) for WLP amount

the liqudiator can repay 800 USD and seize share of 500 USD

but there are 200 USD remaining debt,

when other liquidator (even this liquidator belongs to protocol) writes to repay and erase the rest 200 USD bad debt, he cannot because removeCollateralTo validates share > 0, if share is 0, revert

_require(_shares > 0, Errors.ZERO_VALUE);

if the underlying collateral is WLP, the second liquidation aims to write off bad debt does not work as well because if all WLP is transfered out, calling _harvest and unwrap again is likely to revert

_harvest(_posId, _wLp, _tokenId);
IBaseWrapLp(_wLp).unwrap(_tokenId, _amt, _receiver);

the bad permenantly inflate the totalAssets() in lending pool and inflate the total debt to not let other user borrow from protocol because of the borrow cap checks

also, the lender suffer the loss because if the bad debt is not repaid, the lender that deposit cash into the lending pool is lost

Tools Used

Manual Review

Recommended Mitigation Steps

add a way o handle not fully repaid bad debt after liquidation after the lending pool share or WLP is fully seized

add a function donate to the lending pool to let user supply asset or add a function to socialize the bad debt as loss explicilty

Assessed type

Token-Transfer

Lines of code

https://github.com/code-423n4/2023-12-initcapital/blob/a53e401529451b208095b3af11862984d0b32177/contracts/oracle/Api3OracleReader.sol#L87

Vulnerability details

Impact

API3 oracle timestamp can be set to future timestamp and block API3 Oracle usage to make code revert in underflow

Proof of Concept

In the Api3OracleReader.sol, the code assumes tha the timestamp returned from oracle is always in the past

    function getPrice_e36(address _token) external view returns (uint price_e36) {
        // load and check
        DataFeedInfo memory dataFeedInfo = dataFeedInfos[_token];
        _require(dataFeedInfo.dataFeedId != bytes32(0), Errors.DATAFEED_ID_NOT_SET);
        _require(dataFeedInfo.maxStaleTime != 0, Errors.MAX_STALETIME_NOT_SET);

        // get price and token's decimals
        uint decimals = uint(IERC20Metadata(_token).decimals());
        // return price per token with 1e18 precisions
        // e.g. 1 BTC = 35000 * 1e18 in USD_e18 unit
        (int224 price, uint timestamp) = IApi3ServerV1(api3ServerV1).readDataFeedWithId(dataFeedInfo.dataFeedId);

        // check if the last updated is not longer than the max stale time
        _require(block.timestamp - timestamp <= dataFeedInfo.maxStaleTime, Errors.MAX_STALETIME_EXCEEDED);

        // return as [USD_e36 per wei unit]
        price_e36 = (price.toUint256() * ONE_E18) / 10 ** decimals;
    }

note the check

// e.g. 1 BTC = 35000 * 1e18 in USD_e18 unit
(int224 price, uint timestamp) = IApi3ServerV1(api3ServerV1).readDataFeedWithId(dataFeedInfo.dataFeedId);

// check if the last updated is not longer than the max stale time
_require(block.timestamp - timestamp <= dataFeedInfo.maxStaleTime, Errors.MAX_STALETIME_EXCEEDED);

but if timestamp is greater than block.timestamp, the transaction will revert and block oracle lookup on APi3OracleReader.sol

the relayer on api3 side can update both oracle price timestamp and value,

let us go over how the price is updated in Api3 code

the function readDataFeedWithID basically just read the data from struct _dataFeeds[beaconId], in this function

    /// @notice Reads the data feed with ID
    /// @param dataFeedId Data feed ID
    /// @return value Data feed value
    /// @return timestamp Data feed timestamp
    function _readDataFeedWithId(
        bytes32 dataFeedId
    ) internal view returns (int224 value, uint32 timestamp) {
        DataFeed storage dataFeed = _dataFeeds[dataFeedId];
        (value, timestamp) = (dataFeed.value, dataFeed.timestamp);
        require(timestamp > 0, "Data feed not initialized");
    }

when the relayer update the oracle data, first we are calling proecssBeaconUpdate

note that is a modifier onlyValidateTimestamp

  function processBeaconUpdate(
        bytes32 beaconId,
        uint256 timestamp,
        bytes calldata data
    )
        internal
        onlyValidTimestamp(timestamp)
        returns (int224 updatedBeaconValue)
    {
        updatedBeaconValue = decodeFulfillmentData(data);
        require(
            timestamp > _dataFeeds[beaconId].timestamp,
            "Does not update timestamp"
        );
        _dataFeeds[beaconId] = DataFeed({
            value: updatedBeaconValue,
            timestamp: uint32(timestamp)
        });
    }

the check ensure that only when the timstamp is from more than 1 hour, the update revert

    /// @dev Reverts if the timestamp is from more than 1 hour in the future
    modifier onlyValidTimestamp(uint256 timestamp) virtual {
        unchecked {
            require(
                timestamp < block.timestamp + 1 hours,
                "Timestamp not valid"
            );
        }
        _;
    }

what does this mean?

the timestamp of a oracle can be set to the future within an hour, the relayer does not have to be malicious, it is a normal part of updating data

suppose the current timestamp is 10000

1 hour = 3600 seconds

if the relayer set timestamp to 12000, the price update will go through

but the code

_require(block.timestamp - timestamp <= dataFeedInfo.maxStaleTime, Errors.MAX_STALETIME_EXCEEDED);

will revert when current timestamp is 10001

10001 - 12000 will revert in underflow

Tools Used

manual review

Recommended Mitigation Steps

if the oracle timestamp is comes from the future, the code should consider it not stale

can change the code to

if (block.timestamp > timestamp) {
 _require(block.timestamp - timestamp <= dataFeedInfo.maxStaleTime, Errors.MAX_STALETIME_EXCEEDED);
}

Assessed type

Math

Lines of code

https://github.com/code-423n4/2023-12-initcapital/blob/a53e401529451b208095b3af11862984d0b32177/contracts/core/InitCore.sol#L220 https://github.com/code-423n4/2023-12-initcapital/blob/a53e401529451b208095b3af11862984d0b32177/contracts/core/InitCore.sol#L383 https://github.com/code-423n4/2023-12-initcapital/blob/a53e401529451b208095b3af11862984d0b32177/contracts/lending_pool/LendingPool.sol#L148 https://github.com/code-423n4/2023-12-initcapital/blob/a53e401529451b208095b3af11862984d0b32177/contracts/core/InitCore.sol#L462

Vulnerability details

Impact

admin configuration isAllowedForCollateral(mode, pool) can be bypassed by donating asset to the pool directly

Proof of Concept

In the current implementation,

after user deposit fund into lending pool and mint lending pool shares,

user can call collateralize function to add collateral

    function collateralize(uint _posId, address _pool) public virtual onlyAuthorized(_posId) nonReentrant {
        IConfig _config = IConfig(config);
        // check mode status
        uint16 mode = _getPosMode(_posId);
        _require(_config.getModeStatus(mode).canCollateralize, Errors.COLLATERALIZE_PAUSED);
        // check if the position mode supports _pool
        _require(_config.isAllowedForCollateral(mode, _pool), Errors.INVALID_MODE);
        // update collateral on the position
        uint amtColl = IPosManager(POS_MANAGER).addCollateral(_posId, _pool);
        emit Collateralize(_posId, _pool, amtColl);
    }

there is a validation

 _require(_config.isAllowedForCollateral(mode, _pool), Errors.INVALID_MODE);

let us consider the case:

the mode support three pools, USDC lending pool, WETH lending pool and a token A lending pool

the token A is subject to high volatility, the admin decides to disalllow token A lending pool added as collateral

but then the token A is hacked,

the attacker can mint the token A infnitely

there are relevant hack in the past

1. https://ciphertrace.com/infinite-minting-exploit-nets-attacker-4-4m/

attacker exploit logical and math error to mint token infnitely

2. https://www.coindesk.com/business/2022/10/10/binance-exec-bnb-smart-chain-hack-could-have-been-worse-if-validators-hadnt-sprung-into-action/

attacker exploit cryptographical logic to mint token infinitely

but even when admin disallow a mode from further collaterize or disallow the lending pool from further collaterize

the hacker can donate the token to the lending pool and inflate the share worth to borrow all fund out

1. hacker transfer the infinitely minted token to the lending pool
2. then hacker can call the function flash

this would trigger the function syncCash, which update the cash amount in the lending pool to make sure the donated token count into the token worth

// execute callback
IFlashReceiver(msg.sender).flashCallback(_pools, _amts, fees, _data);
// sync cash
for (uint i; i < _pools.length; i = i.uinc()) {
	uint poolCash = ILendingPool(_pools[i]).syncCash();

sync cash is called

   /// @inheritdoc ILendingPool
    function syncCash() external accrue onlyCore returns (uint newCash) {
        newCash = IERC20(underlyingToken).balanceOf(address(this));
        _require(newCash >= cash, Errors.INVALID_AMOUNT_TO_REPAY); // flash not repay
        cash = newCash;
    }

basically by using the flash function and then trigger syncCash, user can always donate the token to the pool to inflate share worth

then in the collateral credit calculation, we are converting shares worth to amonut worth

            uint tokenValue_e36 = ILendingPool(pools[i]).toAmtCurrent(shares[i]) * tokenPrice_e36;

which calls the funciton toAmt

   function _toAmt(uint _shares, uint _totalAssets, uint _totalShares) internal pure returns (uint amt) {
        return _shares.mulDiv(_totalAssets + VIRTUAL_ASSETS, _totalShares + VIRTUAL_SHARES);
    }
}

assume shares does not change assume total shares does not chang

clearly inflating the total asset (which is _cash + debt)

inflate shares worth

again, in the case of infinite token minting, hacker can donating the token to the lending pool to inflate the collateral credit and borrow all fund out and create large bad debt

Tools Used

Manual Review

Recommended Mitigation Steps

use internal balance to track the cash amount and do not allow user to indirectly access the sync cash function via flash loan

Assessed type

Token-Transfer

Table of Contents

Lack of multicall support in MoneyMarketHook to collaterize WLP or decollaterize WLP

In MoneyMarketHook.sol, there is no function to let user collateralize WLP or decollaterlaize WLP

it is recommend to implement this feature to make sure user can also use WLP as collateral via moneyMarketHook.sol

Inaccurate error message in MoneyMarketHook.sol

in the function execution if the account health is less than specified

_params.minHealth_e18

transaction revert in this line of code

_require(_params.minHealth_e18 <= IInitCore(CORE).getPosHealthCurrent_e18(initPosId), Errors.SLIPPAGE_CONTROL);

However, the error message can be more accurate, the error message can be changed to "ERRORS.HEALTH_FACTOR_TOO_LOW"

lack of function to transfer the position NFT out in MoneyMarketHook.sol

function execute(OperationParams calldata _params)
	external
	payable
	returns (uint posId, uint initPosId, bytes[] memory results)
{
	// create position if not exist
	if (_params.posId == 0) {
		(posId, initPosId) = createPos(_params.mode, _params.viewer);
	} else {
		// for existing position, only owner can execute
		posId = _params.posId;
		initPosId = initPosIds[msg.sender][posId];
		_require(IERC721(POS_MANAGER).ownerOf(initPosId) == address(this), Errors.NOT_OWNER);
	}

in the contract MoneyMarketHook.sol, the user can create a position, and then the owner of the position nft can execute multicall action

however, there is lack of function to transfer the position nft out

the user may want to transfer the nft out to sell it in the secondary marketplace or he may just want to transfer the nft when he wants to use the nft in another account

recommend add the function to let original owner transfer the nft out from MoneyMarketHook.sol

callback function does not validate if msg.value equals to value amount

    /// @inheritdoc IInitCore
    function callback(address _to, uint _value, bytes memory _data)
        public
        payable
        virtual
        returns (bytes memory result)
    {
        _require(_to != address(this), Errors.INVALID_CALLBACK_ADDRESS);
        // call _to with _data
        return ICallbackReceiver(_to).coreCallback{value: _value}(msg.sender, _data);
    }

In this function callback, if the msg.value attached is 1 ETH, but value amount passed in from the function is 0.1 ETH

the rest 0.9 ETH are lost

it is recommend to validate msg.vaue attached in callback equals to the _value amount

_value == msg.vlaue 

should validate the helper address in market hook address

in the MoneyMarketHook.sol contract, the code does not validate the helper address

for (uint i; i < _params.withdrawParams.length; i = i.uinc()) {
	address helper = _params.withdrawParams[i].rebaseHelperParams.helper;
	if (helper != address(0)) IRebaseHelper(helper).unwrap(_params.withdrawParams[i].to);
}

the helper is expected to by USDY rebase helper

so in the code, it is recommend for admin to validate the helper address passed and then only allow user to use whitelisted helper

USDY admin can blocklist / sanction, then address may not transfer USDY to repay / redeem from lending pool

https://etherscan.io/address/0xea0f7eebdc2ae40edfe33bf03d332f8a7f617528#code#F1#L94

USDY admin can add account to blocklist or sanction list then the user that

use UDSY as collateral may not able to use USDY to mint more lending pool share or burn lending pool share to redeem USDY

Remove WLP from whitelist should not block user from removing WLP

when user use WLP as collateral to collateralize the position

// check if the wLp is whitelisted
_require(_config.whitelistedWLps(_wLp), Errors.TOKEN_NOT_WHITELISTED);

we are checking if the WLP is whitelisted

but when admin remove WLP from whitelist, the user should not use WLP to further collateralize the position,

but when decollateralize and remove the WLP, the same check applies

// check if the wLp is whitelisted
_require(_config.whitelistedWLps(_wLp), Errors.TOKEN_NOT_WHITELISTED);

so when admin remove WLP from whitelist, the user cannot decollateralize WLP as well

recommendation is do not let WLP unwhitlisting block decollateraliztion

Missing price deviation check when there is only one valid oracle

In Oracle code

// normal case: both sources are valid
// check that the prices are not too deviated
// abnormal case: one of the sources is invalid
// using the valid source - prioritize the primary source
// abnormal case: both sources are invalid
// revert
_require(isPrimarySourceValid || isSecondarySourceValid, Errors.NO_VALID_SOURCE);
if (isPrimarySourceValid && isSecondarySourceValid) {
	// sort Price
	(uint minPrice_e36, uint maxPrice_e36) = primaryPrice_e36 < secondaryPrice_e36
		? (primaryPrice_e36, secondaryPrice_e36)
		: (secondaryPrice_e36, primaryPrice_e36);

	// check deviation
	_require(
		(maxPrice_e36 * ONE_E18) / minPrice_e36 <= maxPriceDeviations_e18[_token], Errors.TOO_MUCH_DEVIATION
	);
}
price_e36 = isPrimarySourceValid ? primaryPrice_e36 : secondaryPrice_e36;

when there are two valid price source, the code cross-validates the price deviation,

However, when there is only one valid price, the code consumes the price without checking the price deviation

it is recommended to cache the last price and validate if the new price deviates from the last cached price too much