Venus Protocol Isolated Pools - 0x8chars's results

Lines of code

https://github.com/code-423n4/2023-05-venus/blob/main/contracts/MaxLoopsLimitHelper.sol#L39-L42 https://github.com/code-423n4/2023-05-venus/blob/main/contracts/MaxLoopsLimitHelper.sol#L5

Vulnerability details

Impact

According to the comment on line 5, the _ensureMaxLoop() function is used to avoid DOS but a malicious user can abuse this by ensuring that his account cannot be liquidated.

Proof of Concept

Assume that maxLoopLimit is 20 and there are more than 20 pools. A malicious user simply needs to deposit 1 token into each pool. When someone tries to call liquidateAccount() with his address and an array of orders. If the liquidator submits more than 20 orders, line 667 will fail. If the user tries to submit less than 20 orders, the loop on line 690-693 will fail.

Recommended Mitigation Steps

Remove this check because there are only 2 public functions that call _ensureMaxLoop internally. enterMarkets() doesn’t have any side effects while the other function where _ensureMaxLoop can be abused is in the liquidateAccount function. The other functions that call _ensureMaxLoop are all gated by access control.

Assessed type

Loop

Lines of code

https://github.com/code-423n4/2023-05-venus/blob/main/contracts/VToken.sol#L1476-L1478

Vulnerability details

Impact

By minting 1 wei of token (actualMintAmount is initialExchangeRateMantissa unscaled by 1e18) and transferring the underlying directly to the vToken contract, it will inflate the way exchangeRate is calculated. This has the same underlying issue as the following:

• https://github.com/code-423n4/2022-06-canto-findings/issues/162
• https://github.com/code-423n4/2022-03-prepo-findings/issues/27
• yVault: First depositor can break minting of shares · Issue #12 · code-423n4/2022-04-jpegd-findings (github.com)

Proof of Concept

function _exchangeRateStored() internal view virtual returns (uint256) {
    uint256 _totalSupply = totalSupply;
    if (_totalSupply == 0) {
        /*
         * If there are no tokens minted:
         *  exchangeRate = initialExchangeRate
         */
        return initialExchangeRateMantissa;
    } else {
        /*
         * Otherwise:
         *  exchangeRate = (totalCash + totalBorrows + badDebt - totalReserves) / totalSupply
         */
        uint256 totalCash = _getCashPrior();
        uint256 cashPlusBorrowsMinusReserves = totalCash + totalBorrows + badDebt - totalReserves;
        uint256 exchangeRate = (cashPlusBorrowsMinusReserves * expScale) / _totalSupply;

        return exchangeRate;
    }
}

Since 1 wei is minted, totalSupply is no longer 0 therefore it will calculate the exchangeRate. totalCash() returns the current balance of the underlying tokens in the vToken contract (which is inflated due to malicious user transferring tokens directly to the vToken contract).

cashPlusBorrowsMinusReserves is calculated as inflated value + 0 + 0 - 0 i.e. inflated value.

exchangeRate is calculated as inflated value * 1e18 / 1 i.e. inflated value.

Therefore, all subsequent mints will need to mint at a much higher rate than the first minter. If subsequent mints are not large enough, it will only inflate the exchangeRate further.

Recommended Mitigation Steps

After line 776, check that mintTokens cannot be 0.

Assessed type

DoS