INIT Capital Invitational - 0x73696d616f's results

Lines of code

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

Vulnerability details

Impact

Users can avoid being liquidated if they frontrun liquidation calls with a liquidate call with 1 wei. Or, they may do a partial liquidation and avoid being liquidated before the interest reaches the value of the debt pre liquidation. The total interest stored in __posBorrInfos[_posId].borrExtraInfos[_pool].totalInterest would also be wrong.

Proof of Concept

The POS_MANAGER stores the total interest in __posBorrInfos[_posId].borrExtraInfos[_pool].totalInterest. Function updatePosDebtShares() assumes that ILendingPool(_pool).debtShareToAmtCurrent(currDebtShares) is always increasing, but this is not the case, as a liquidation may happen that reduces the current debt amount. This leads to calls to updatePosDebtShares() reverting.

The most relevant is when liquidating, such that users could liquidate themselves for small amounts (1) and prevent liqudiations in the same block. This is because the debt accrual happens over time, so if the block.timestamp is the same, no debt accrual will happen. Thus, if a liquidate call with 1 amount frontruns a liquidate call with any amount, the second call will revert.

A user could still stop liquidations for as long as the accrued interest doesn't reach the last debt value before liquidation, if the user liquidated a bigger part of the debt.

Add the following test to TestInitCore.sol:

function test_POC_Liquidate_reverts_frontrunning_PosManager_WrongAssumption() public {
    address poolUSDT = address(lendingPools[USDT]);
    address poolWBTC = address(lendingPools[WBTC]);
    _setTargetHealthAfterLiquidation_e18(1, type(uint64).max); // by pass max health after liquidate capped
    _setFixedRateIRM(poolWBTC, 0.1e18); // 10% per sec

    uint collAmt;
    uint borrAmt;

    {
        uint collUSD = 100_000;
        uint borrUSDMax = 80_000;
        collAmt = _priceToTokenAmt(USDT, collUSD);
        borrAmt = _priceToTokenAmt(WBTC, borrUSDMax);
    }

    address liquidator = BOB;
    deal(USDT, ALICE, collAmt);
    deal(WBTC, liquidator, borrAmt * 2);

    // provides liquidity for borrow
    _fundPool(poolWBTC, borrAmt);

    // create position and collateralize
    uint posId = _createPos(ALICE, ALICE, 1);
    _collateralizePosition(ALICE, posId, poolUSDT, collAmt, bytes(''));

    // borrow
    _borrow(ALICE, posId, poolWBTC, borrAmt, bytes(''));

    // fast forward time and accrue interest
    vm.warp(block.timestamp + 1 seconds);
    ILendingPool(poolWBTC).accrueInterest();

    uint debtShares = positionManager.getPosDebtShares(posId, poolWBTC);

    _liquidate(liquidator, posId, 1, poolWBTC, poolUSDT, false, bytes(''));

    // liquidate all debtShares
    _liquidate(liquidator, posId, 1000, poolWBTC, poolUSDT, false, bytes('panic'));
}

Tools Used

Vscode, Foundry

Recommended Mitigation Steps

Update the user's last debt position __posBorrInfos[_posId].borrExtraInfos[_pool].totalInterest on _repay().

Assessed type

Under/Overflow

Lines of code

https://github.com/code-423n4/2023-12-initcapital/blob/main/contracts/core/InitCore.sol#L151 https://github.com/code-423n4/2023-12-initcapital/blob/main/contracts/core/InitCore.sol#L282 https://github.com/code-423n4/2023-12-initcapital/blob/main/contracts/core/InitCore.sol#L317

Vulnerability details

Impact

repay(), liquidate() and liquidateWLp() transactions revert if users approve the exact repay amount they need in the frontend and only after some blocks have passed is the transaction settled. This happens because the interest accrual is by timestamp, so the debt would have increased since the approval, when the transaction settles.

Proof of Concept

A test when repaying debt was carried out in TestInitCore.sol. The timestamp increased just 1 second, but it was enough to make the transaction revert. It may be possible to request a bigger alowance than expected, but this has other implications.

function test_POC_TransferFromFails_DueToDebtAccrual() public {
    uint256 _wbtcAmount = 3e8;
    uint256 _borrowAmount = 1e8;
    address _user = makeAddr("user");
    deal(WBTC, _user, _wbtcAmount);
    
    uint256 _posId = _createPos(_user, _user, 2);
    uint256 shares_ = _mintPool(_user, address(lendingPools[WBTC]), _wbtcAmount, "");
    vm.startPrank(_user);
    lendingPools[WBTC].transfer(address(positionManager), shares_);
    initCore.collateralize(_posId, address(lendingPools[WBTC]));
    vm.stopPrank();

    uint256 _debtShares = _borrow(_user, _posId, address(lendingPools[WBTC]), _borrowAmount, "");

    uint256 _userDebtBalance = lendingPools[WBTC].debtShareToAmtCurrent(_debtShares);

    vm.prank(_user);
    IERC20(WBTC).approve(address(initCore), _userDebtBalance);

    skip(1); 

    vm.prank(_user);
    vm.expectRevert("ERC20: transfer amount exceeds balance");
    initCore.repay(address(lendingPools[WBTC]), _debtShares, _posId);
}

Tools Used

Vscode, Foundry

Recommended Mitigation Steps

Receive the amount in InitCore as argument instead of the shares on the repay(), liquidate() and liquidateWLp() functions.

Assessed type

Under/Overflow

Lines of code

https://github.com/code-423n4/2023-12-initcapital/blob/main/contracts/lending_pool/LendingPool.sol#L95-L97

Vulnerability details

Impact

The impact of this finding is more on the marketing/data fetching side, on exchanges it would appear that the shares are worth less VIRTUAL_SHARES than the underlying token. Given that it would influence the perception of the value of the shares token, medium severity seems appropriate.

Proof of Concept

The Openzeppelin implementation includes the decimals offset (log10(VIRTUAL_SHARES) in LendingPool) in the decimals() function. However, INIT only places the decimals of the underlying.

A POC was built, add it to TestLendingPool.sol:

function test_POC_WrongDecimals() public {
    uint256 _wbtcAmount = 3e8; // 3 BTC
    address _user = makeAddr("user");
    _mintPool(_user, WBTC, _wbtcAmount);
    uint256 _wbtcDecimals = 1e8;
    uint256 VIRTUAL_SHARES = 1e8;
    uint256 _poolDecimals = 10**lendingPools[WBTC].decimals();
    uint256 _userBalance = lendingPools[WBTC].balanceOf(_user);
    assertEq(_userBalance/_poolDecimals, _wbtcAmount/_wbtcDecimals*VIRTUAL_SHARES);
    assertEq(_userBalance/_poolDecimals, 3e8);
    assertEq(_userBalance, _wbtcAmount*VIRTUAL_SHARES);
}

Tools Used

Vscode, Foundry

Recommended Mitigation Steps

Include the virtual shares decimals in the decimals() function:

uint private constant VIRTUAL_SHARES = 8;
...
function decimals() public view override returns (uint8) {
    return IERC20Metadata(underlyingToken).decimals() + VIRTUAL_SHARES;
}
...
function _toShares(uint _amt, uint _totalAssets, uint _totalShares) internal pure returns (uint shares) {
    return _amt.mulDiv(_totalShares + 10**VIRTUAL_SHARES, _totalAssets + VIRTUAL_ASSETS);
}
...
function _toAmt(uint _shares, uint _totalAssets, uint _totalShares) internal pure returns (uint amt) {
    return _shares.mulDiv(_totalAssets + VIRTUAL_ASSETS, _totalShares + 10**VIRTUAL_SHARES);
}

Assessed type

ERC20

Lines of code

https://github.com/code-423n4/2023-12-initcapital/blob/main/contracts/core/InitCore.sol#L327

Vulnerability details

Impact

Users who collateralized using wLP won't ever be liquidated unless the wLP is whitelisted back, but this could be dangerous depending on the reason it was removed from the whitelist. They can't also decollateralize. The severity of this issue depends entirely on the reason of the removal.

Proof of Concept

Add the following code before this line:

address[] memory wlps = new address[](1);
wlps[0] = address(mockWLpUniV2);
config.setWhitelistedWLps(wlps, false);

The test will fail due to the fact that the wLP is no longer whitelisted, due to the check here.

Tools Used

Vscode, Foundry

Recommended Mitigation Steps

2 different allowlists should be set, one for collateralizing and another for decollateralizing/liquidating. This way, a WLP can be removed from collateralizing only, being it still possible to decollateralize/liquidate. In case the wLP is compromised or similar, it can be removed from both whitelists.

Assessed type

Access Control

Lines of code

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

Vulnerability details

Impact

Oracle unavailable for up to 1 hour, which could lead to positions going underwater and being liquidated when it becomes live again, without users having the chance to repay/collateralize.

Proof of Concept

The airnode code has a note concerning this behaviour: /// @dev Reverts if the timestamp is from more than 1 hour in the future It means that the prices can be up to 1 hour in the future.

This modifier is triggered when updating prices in processBeaconUpdate(), setting the dataFeed, which is read in _readDataFeedWithId(), called initially in readDataFeedWithId().

Then, in the Api3OracleReader, the following check may underflow if the timestamp is in the future: _require(block.timestamp - timestamp <= dataFeedInfo.maxStaleTime, Errors.MAX_STALETIME_EXCEEDED);.

Tools Used

Vscode Foundry

Recommended Mitigation Steps

Limit the timestamp to be at most block.timestamp.

Assessed type

Under/Overflow

[L - 01] execute() could check leftover balances in all interacted tokens.

Users could make mistakes and tokens beside ETH could be left in the contract. The refund mechanism could be extended to other tokens as well.

[L - 02] reserveFactor in LendingPool should be capped at 1e18

setReserveFactor()_e18 does not check the reserveFactor, which if bigger than 1e18, will make accrueInterest() always underflow, DoSing the lending pool functions that depend on the accrue() modifier. It should ideally be capped at a lower value, but this is more of a centralization risk.

[L - 03] missing a way to remove collTokens from Config.sol, which could be dangerous in the long run as some token could go rogue (or an upgrade).

[L - 04] setBorrFactors_e18() could check for duplicate _pools as an additional check to make sure that no incorrect factors are set. If 2 duplicates are sent, only the latter will take effect, which could have very dangerous implications.

[L - 05] excess ETH in InitCore:Multicall() and [InitCore:callback() ](https://github.com/code-423n4/2023-12-initcapital/blob/main/contracts/core/InitCore.sol#L505) could be refunded.

[L - 06] [_liquidateInternal() ](https://github.com/code-423n4/2023-12-initcapital/blob/main/contracts/core/InitCore.sol#L592) should revert if the mode is 0.

[L - 07] msg.value in a loop reverts or steals balance from the contract (although it is not supposed to hold funds). It's still dangerous anyway.