Venus Protocol Isolated Pools - 0xkazim's results

Lines of code

https://github.com/code-423n4/2023-05-venus/blob/8be784ed9752b80e6f1b8b781e2e6251748d0d7e/contracts/BaseJumpRateModelV2.sol#L23

Vulnerability details

Impact

The current implementation of the protocol uses blocks rather than seconds to measure time between interest accruals. This makes the implementation highly sensitive to changes in the average time between Ethereum blocks.

Proof of Concept

On line 23 of BaseJumpRateModelV2 it is implicitly assumed that the time between blocks is 3 seconds(bnb chain need 3 seconds to mine block). However, the average time between blocks can change dramatically. For example, the average time between blocks may increase by significant factors due to the difficulty bomb or decrease by significant factors during the transition to Serenity. The difference between the actual time between blocks and the assumed time between blocks causes proportional differences between the intended interest rates and the actual interest rates. While it is possible for the admin to combat this by adjusting the interest rate model when the average time between blocks changes, such adjustments are manual and happen only after-the-fact. Errors in blocktime assumptions are cumulative, and fixing the model after-the-fact does not make users whole – it only prevents incorrect interest calculations moving forward (until the next change in blocktime).

https://github.com/code-423n4/2023-05-venus/blob/8be784ed9752b80e6f1b8b781e2e6251748d0d7e/contracts/BaseJumpRateModelV2.sol#L23

similar case was submitted by OZ team when they auditing compound protocol : https://blog.openzeppelin.com/compound-audit/

Tools Used

manual review / solodit(reviewing OZ reports)

Recommended Mitigation Steps

Consider refactoring the implementation to use seconds rather than blocks to measure the time between accruals. While block.timestamp can be manipulated by miners within a narrow window, these errors are small and, importantly, are not cumulative. This would decouple the interest rate model from Ethereum’s average blocktime.

Assessed type

Timing

Lines of code

https://github.com/code-423n4/2023-05-venus/blob/8be784ed9752b80e6f1b8b781e2e6251748d0d7e/contracts/WhitePaperInterestRateModel.sol#L17

Vulnerability details

Impact

the blocksPerYear is set to 2102400 in the WhitePaperInterestRateModel this should be equal to number of blocks per year that is assumed by the interest rate model, but the number of block is set incorrectly and it's not equal to block per year.

Proof of Concept

in the contract WhitePaperInterestRateModel the block per year is set to 2102400 same as the compound protocol do, but this won't work as it's for the compound protocol, this number will be equal to block number per year only if the contract/protocol is deploying on Ethereum blockchain because it need 15-13 seconds to mine a block which 2102400 will be equal to 31,536,000 second or 365 days(compound v2 is deployed when ethereum mining time for one block was15 seconds) but this will be 73 days on bnb chain because bnb need 3 seconds to mine a block which mean that 2102400 blocks will be equal to 6,307,200 seconds or 73 days( not one year) this will cause a may problem into the protocol because it set block per 73 days not 365 days and the WhitePaperInterestRateModelFactory will create WhitePaperInterestRateModel depending on this informations.

Tools Used

manual review

Recommended Mitigation Steps

i recommend to set the correct block number which is 10512000 that equal to 31,536,000 seconds or 365 days, this block number is set in the BaseJumpRateModelV2 contract so i recommend to do the same thing in the WhitePaperInterestRateModel

Assessed type

Timing

Lines of code

https://github.com/code-423n4/2023-05-venus/blob/8be784ed9752b80e6f1b8b781e2e6251748d0d7e/contracts/RiskFund/RiskFund.sol#L151-L182 https://github.com/code-423n4/2023-05-venus/blob/8be784ed9752b80e6f1b8b781e2e6251748d0d7e/contracts/RiskFund/RiskFund.sol#L235

Vulnerability details

Impact

when a call made to the swapPoolsAssets function this function will call the _swapAsset function to doing the swap, if you check this function you will recognized that the balanceOfUnderlyingAsset is set before the update of the address(vToken) which may cause using a old price than it expected.

ComptrollerViewInterface(comptroller).oracle().updatePrice(address(vToken));

Proof of Concept

when the swapPoolsAssets function is called this function will call the _swapAsset function

function swapPoolsAssets(
        address[] calldata markets,
        uint256[] calldata amountsOutMin,
        address[][] calldata paths
    ) external override returns (uint256) {
        _checkAccessAllowed("swapPoolsAssets(address[],uint256[],address[][])");
        require(poolRegistry != address(0), "Risk fund: Invalid pool registry.");
        require(markets.length == amountsOutMin.length, "Risk fund: markets and amountsOutMin are unequal lengths");
        require(markets.length == paths.length, "Risk fund: markets and paths are unequal lengths");

        uint256 totalAmount;
        uint256 marketsCount = markets.length;

        _ensureMaxLoops(marketsCount);

        for (uint256 i; i < marketsCount; ++i) {
            VToken vToken = VToken(markets[i]);
            address comptroller = address(vToken.comptroller());

            PoolRegistry.VenusPool memory pool = PoolRegistry(poolRegistry).getPoolByComptroller(comptroller);
            require(pool.comptroller == comptroller, "comptroller doesn't exist pool registry");
            require(Comptroller(comptroller).isMarketListed(vToken), "market is not listed");

            uint256 swappedTokens = _swapAsset(vToken, comptroller, amountsOutMin[i], paths[i]);
            poolReserves[comptroller] = poolReserves[comptroller] + swappedTokens;
            totalAmount = totalAmount + swappedTokens;
        }

        emit SwappedPoolsAssets(markets, amountsOutMin, totalAmount);

        return totalAmount;
    }

https://github.com/code-423n4/2023-05-venus/blob/8be784ed9752b80e6f1b8b781e2e6251748d0d7e/contracts/RiskFund/RiskFund.sol#L151-L182

2-this function will all the _swapAsset function to make the swap as it show in the code :

function _swapAsset(
        VToken vToken,
        address comptroller,
        uint256 amountOutMin,
        address[] calldata path
    ) internal returns (uint256) {
        //audit 
        require(amountOutMin != 0, "RiskFund: amountOutMin must be greater than 0 to swap vToken");
        require(amountOutMin >= minAmountToConvert, "RiskFund: amountOutMin should be greater than minAmountToConvert");
        uint256 totalAmount;


        //@audit-info updatePrice should be updated before 

        address underlyingAsset = VTokenInterface(address(vToken)).underlying();
        uint256 balanceOfUnderlyingAsset = poolsAssetsReserves[comptroller][underlyingAsset];

        ComptrollerViewInterface(comptroller).oracle().updatePrice(address(vToken));

        uint256 underlyingAssetPrice = ComptrollerViewInterface(comptroller).oracle().getUnderlyingPrice(
            address(vToken)
        );

        if (balanceOfUnderlyingAsset > 0) {
            Exp memory oraclePrice = Exp({ mantissa: underlyingAssetPrice });
            uint256 amountInUsd = mul_ScalarTruncate(oraclePrice, balanceOfUnderlyingAsset);

            if (amountInUsd >= minAmountToConvert) {
                assetsReserves[underlyingAsset] -= balanceOfUnderlyingAsset;
                poolsAssetsReserves[comptroller][underlyingAsset] -= balanceOfUnderlyingAsset;

                if (underlyingAsset != convertibleBaseAsset) {
                    require(path[0] == underlyingAsset, "RiskFund: swap path must start with the underlying asset");
                    require(
                        path[path.length - 1] == convertibleBaseAsset,
                        "RiskFund: finally path must be convertible base asset"
                    );
                    IERC20Upgradeable(underlyingAsset).safeApprove(pancakeSwapRouter, 0);
                    IERC20Upgradeable(underlyingAsset).safeApprove(pancakeSwapRouter, balanceOfUnderlyingAsset);
                    uint256[] memory amounts = IPancakeswapV2Router(pancakeSwapRouter).swapExactTokensForTokens(
                        balanceOfUnderlyingAsset,
                        amountOutMin,
                        path,
                        address(this),
                        block.timestamp
                    );
                    totalAmount = amounts[path.length - 1];
                } else {
                    totalAmount = balanceOfUnderlyingAsset;
                }
            }
        }

        return totalAmount;
    }

https://github.com/code-423n4/2023-05-venus/blob/8be784ed9752b80e6f1b8b781e2e6251748d0d7e/contracts/RiskFund/RiskFund.sol#LL225C4-L276C2

3-as you can see the function is using balanceOfUnderlyingAsset in many places in the function,for example it used for calculating usdAmount. the problem is that the balanceOfUnderlyingAsset is set before the oracle price update, this will cause to use a old price than the oracle price and the time between them is on of the reason to allow this problem to happeen, the function is using balanceOfUnderlyingAsset which is set before the update happen:

        address underlyingAsset = VTokenInterface(address(vToken)).underlying();
        uint256 balanceOfUnderlyingAsset = poolsAssetsReserves[comptroller][underlyingAsset];

        ComptrollerViewInterface(comptroller).oracle().updatePrice(address(vToken));

as you can see the balance for the underlying asset will set before updating the address(vToken) which is used in the underlyingAsset, this may cause of using old price/balance in this function.

Tools Used

manual review

Recommended Mitigation Steps

always recommended to doing the oracle updates and price updates before setting any balance/price.

        ComptrollerViewInterface(comptroller).oracle().updatePrice(address(vToken));
        address underlyingAsset = VTokenInterface(address(vToken)).underlying();
        uint256 balanceOfUnderlyingAsset = poolsAssetsReserves[comptroller][underlyingAsset];

Assessed type

Oracle

Findings Summary

[G-01] unncessary set of unnessasry exchange rate to memory

Description

there is no need to set the exchange rate to memory in the preMintHook in the comptroller contract because we add it then into storage again.

context

file: contract/shortfall.shorFall.sol

 function preMintHook(
        address vToken,
        address minter,
        uint256 mintAmount
    ) external override {
        _checkActionPauseState(vToken, Action.MINT);

        if (!markets[vToken].isListed) {
            revert MarketNotListed(address(vToken));
        }

        uint256 supplyCap = supplyCaps[vToken];
        // Skipping the cap check for uncapped coins to save some gas
        if (supplyCap != type(uint256).max) {
            uint256 vTokenSupply = VToken(vToken).totalSupply();
            //@audit unnessasry exchange rate !
            Exp memory exchangeRate = Exp({ mantissa: VToken(vToken).exchangeRateStored() });
            uint256 nextTotalSupply = mul_ScalarTruncateAddUInt(exchangeRate, vTokenSupply, mintAmount);
            if (nextTotalSupply > supplyCap) {
                revert SupplyCapExceeded(vToken, supplyCap);
            }
        }
        ...

Recommendations

recommend to not set the exchangeRate into memory

[l-02] adding max Loop limit to the preMintHook function to avoid DOS if it have possibility to happen

Description

adding _ensureMaxLoops will be a good choice to the preMintHook function to avoid any DOS that may happen to the function where there is a lot of call into mints functions

context

function preMintHook( address vToken, address minter, uint256 mintAmount ) external override { _checkActionPauseState(vToken, Action.MINT);

    if (!markets[vToken].isListed) {
        revert MarketNotListed(address(vToken));
    }

    uint256 supplyCap = supplyCaps[vToken];
    // Skipping the cap check for uncapped coins to save some gas
    if (supplyCap != type(uint256).max) {
        uint256 vTokenSupply = VToken(vToken).totalSupply();
        Exp memory exchangeRate = Exp({ mantissa: VToken(vToken).exchangeRateStored() });
        uint256 nextTotalSupply = mul_ScalarTruncateAddUInt(exchangeRate, vTokenSupply, mintAmount);
        if (nextTotalSupply > supplyCap) {
            revert SupplyCapExceeded(vToken, supplyCap);
        }
    }

    // Keep the flywheel moving
    uint256 rewardDistributorsCount = rewardsDistributors.length;
    //audit add max loop here !

    for (uint256 i; i < rewardDistributorsCount; ++i) {
        rewardsDistributors[i].updateRewardTokenSupplyIndex(vToken);
        rewardsDistributors[i].distributeSupplierRewardToken(vToken, minter);
    }
}

Recommendations

add _ensureMaxLoops to this function to avoid DOS in some cases.