code-423n4 / 2023-05-venus-findings

Venus Protocol Contest

Unless otherwise discussed, this repo will be made public after contest completion, sponsor review, judging, and two-week issue mitigation window.

Contributors to this repo: prior to report publication, please review the Agreements & Disclosures issue.

---

Contest findings are submitted to this repo

Sponsors have three critical tasks in the contest process:

1. Weigh in on severity.
2. Respond to issues.
3. Share your mitigation of findings.

Let's walk through each of these.

High and Medium Risk Issues

Please note: because wardens submit issues without seeing each other's submissions, there will always be findings that are duplicates. For all issues labeled 3 (High Risk) or 2 (Medium Risk), these have been pre-sorted for you so that there is only one primary issue open per unique finding. All duplicates have been labeled duplicate, linked to a primary issue, and closed.

Weigh in on severity

Judges have the ultimate discretion in determining severity of issues, as well as whether/how issues are considered duplicates. However, sponsor input is a significant criteria.

For a detailed breakdown of severity criteria and how to estimate risk, please refer to the judging criteria in our documentation.

If you disagree with a finding's severity, leave the severity label intact and add the label disagree with severity, along with a comment indicating your opinion for the judges to review. You may also add questions for the judge in the comments.

Respond to issues

Label each open/primary High or Medium risk finding as one of these:

• sponsor confirmed, meaning: "Yes, this is a problem and we intend to fix it."
• sponsor disputed, meaning either: "We cannot duplicate this issue" or "We disagree that this is an issue at all."
• sponsor acknowledged, meaning: "Yes, technically the issue is correct, but we are not going to resolve it for xyz reasons."

(Note: please don't use sponsor disputed for a finding if you think it should be considered of lower or higher severity. Instead, use the label disagree with severity and add comments to recommend a different severity level -- and include your reasoning.)

Add any necessary comments explaining your rationale for your evaluation of the issue.

Note that when the repo is public, after all issues are mitigated, wardens will read these comments; they may also be included in your C4 audit report.

QA and Gas Reports

For low and non-critical findings (AKA QA), as well as gas optimizations: wardens are required to submit these as bulk listings of issues and recommendations. They may only submit a single, compiled report in each category:

• QA reports should include all low severity and non-critical findings, along with a summary statement.
• Gas reports should include all gas optimization recommendations, along with a summary statement.

For QA and Gas reports, sponsors are not required to weigh in on severity or risk level. We ask that sponsors:

• Leave a comment for the judge on any reports you consider to be particularly high quality. (These reports will be awarded on a curve.)
• Add the sponsor disputed label to any reports that you think should be completely disregarded by the judge, i.e. the report contains no valid findings at all.

Once labelling is complete

When you have finished labelling findings, drop the C4 team a note in your private Discord backroom channel and let us know you've completed the sponsor review process. At this point, we will pass the repo over to the judge to review your feedback while you work on mitigations.

Share your mitigation of findings

Note: this section does not need to be completed in order to finalize judging. You can continue work on mitigations while the judge finalizes their decisions and even beyond that. Ultimately we won't publish the final audit report until you give us the OK.

For each finding you have confirmed, you will want to mitigate the issue before the contest report is made public.

As you undertake that process, we request that you take the following steps:

1. Within a repo in your own GitHub organization, create a pull request for each finding.
2. Link the PR to the issue that it resolves within your contest findings repo.

This will allow for complete transparency in showing the work of mitigating the issues found in the contest. If the issue in question has duplicates, please link to your PR from the open/primary issue.

2023-05-venus-findings's Issues

Gas Optimizations

See the markdown file with the details of this report here.

Gas Optimizations

See the markdown file with the details of this report here.

Gas Optimizations

See the markdown file with the details of this report here.

TEST

Lines of code

https://github.com/code4rena-dev/code4rena.com/blob/c00364df165549efa4fdd6ee657ecc576d2cee44/.gitignore#L1

Vulnerability details

Impact

Detailed description of the impact of this finding.

Proof of Concept

Provide direct links to all referenced code in GitHub. Add screenshots, logs, or any other relevant proof that illustrates the concept.

Tools Used

Recommended Mitigation Steps

Gas Optimizations

See the markdown file with the details of this report here.

QA Report

See the markdown file with the details of this report here.

QA Report

See the markdown file with the details of this report here.

First Deposit Bug

Lines of code

https://github.com/compound-finance/compound-protocol/blob/a3214f67b73310d547e00fc578e8355911c9d376/contracts/CToken.sol#L293

Vulnerability details

First Deposit Bug

The CToken is a yield bearing asset which is minted when any user deposits some units of
underlying tokens. The amount of CTokens minted to a user is calculated based upon
the amount of underlying tokens user is depositing.

As per the implementation of CToken contract, there exist two cases for CToken amount calculation:

1. First deposit - when VToken.totalSupply() is 0.
2. All subsequent deposits.

Here is the actual CToken code (extra code and comments clipped for better reading):

    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;
        }
    }

    function _mintFresh(
        address payer,
        address minter,
        uint256 mintAmount
    ) internal {
        /* Fail if mint not allowed */
        comptroller.preMintHook(address(this), minter, mintAmount);

        /* Verify market's block number equals current block number */
        if (accrualBlockNumber != _getBlockNumber()) {
            revert MintFreshnessCheck();
        }

        Exp memory exchangeRate = Exp({ mantissa: _exchangeRateStored() });
...

/contracts/VToken.sol#L1463

Impact

A sophisticated attack can impact all user deposits until the lending protocols owners and users are notified and contracts are paused. Since this attack is a replicable attack it can be performed continuously to steal the deposits of all depositors that try to deposit into the CToken contract.

The loss amount will be the sum of all deposits done by users into the CToken multiplied by the underlying token's price.

Suppose there are 10 users and each of them tries to deposit 1,000,000 underlying tokens into the CToken contract. Price of underlying token is $1.

Total loss (in $) = $10,000,000

The Fix

The fix to prevent this issue would be to enforce a minimum deposit that cannot be withdrawn. This can be done by minting small amount of CToken units to 0x00 address on the first deposit.

Instead of a fixed 1000 value an admin controlled parameterized value can also be used to control the burn amount on a per CToken basis.

Assessed type

Other

blocksPerYear is not sync like it supposed to in bsc chain

Lines of code

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

Vulnerability details

Impact

Detailed description of the impact of this finding.
The variable calculations in WhitePaperInterestRateModel are incorrect compared to BaseJumpRateModelV2 due to a lack of synchronization in blocksPerYear.

Proof of Concept

Provide direct links to all referenced code in GitHub. Add screenshots, logs, or any other relevant proof that illustrates the concept.
blocksPerYear should be 10512000 just like in WhitePaperInterestRateModel for bsc chain

    /**
     * @notice The approximate number of blocks per year that is assumed by the interest rate model
     */
    uint256 public constant blocksPerYear = 2102400;

contracts/WhitePaperInterestRateModel.sol#L17

    /**
     * @notice The approximate number of blocks per year that is assumed by the interest rate model
     */
    uint256 public constant blocksPerYear = 10512000;

contracts/BaseJumpRateModelV2.sol#L23

There will be invalid baseRatePerBlock and multiplierPerBlock

    constructor(uint256 baseRatePerYear, uint256 multiplierPerYear) {
        baseRatePerBlock = baseRatePerYear / blocksPerYear;
        multiplierPerBlock = multiplierPerYear / blocksPerYear;

        emit NewInterestParams(baseRatePerBlock, multiplierPerBlock);
    }

contracts/WhitePaperInterestRateModel.sol#L36

Tools Used

Manual

Recommended Mitigation Steps

Change to 10512000

Assessed type

Invalid Validation

QA Report

See the markdown file with the details of this report here.

QA Report

See the markdown file with the details of this report here.

QA Report

See the markdown file with the details of this report here.

Sometimes calculateBorrowerReward and calculateSupplierReward return incorrect results

Lines of code

https://github.com/code-423n4/2023-05-venus/blob/9853f6f4fe906b635e214b22de9f627c6a17ba5b/contracts/Lens/PoolLens.sol#L506

Vulnerability details

Impact

Detailed description of the impact of this finding.
Sometimes calculateBorrowerReward and calculateSupplierReward return incorrect results

Proof of Concept

Provide direct links to all referenced code in GitHub. Add screenshots, logs, or any other relevant proof that illustrates the concept.
Whenever user wants to know his pending rewards he calls getPendingRewards sometimes it returns incorrect results.

There is a bug inside calculateBorrowerReward and calculateSupplierReward

    function calculateBorrowerReward(
        address vToken,
        RewardsDistributor rewardsDistributor,
        address borrower,
        RewardTokenState memory borrowState,
        Exp memory marketBorrowIndex
    ) internal view returns (uint256) {
        Double memory borrowIndex = Double({ mantissa: borrowState.index });
        Double memory borrowerIndex = Double({
            mantissa: rewardsDistributor.rewardTokenBorrowerIndex(vToken, borrower)
        });
//      @audit
//        if (borrowerIndex.mantissa == 0 && borrowIndex.mantissa >= rewardsDistributor.rewardTokenInitialIndex()) {
        if (borrowerIndex.mantissa == 0 && borrowIndex.mantissa > 0) {
            // Covers the case where users borrowed tokens before the market's borrow state index was set
            borrowerIndex.mantissa = rewardsDistributor.rewardTokenInitialIndex();
        }
        Double memory deltaIndex = sub_(borrowIndex, borrowerIndex);
        uint256 borrowerAmount = div_(VToken(vToken).borrowBalanceStored(borrower), marketBorrowIndex);
        uint256 borrowerDelta = mul_(borrowerAmount, deltaIndex);
        return borrowerDelta;
    }

contracts/Lens/PoolLens.sol#L495

    function calculateSupplierReward(
        address vToken,
        RewardsDistributor rewardsDistributor,
        address supplier,
        RewardTokenState memory supplyState
    ) internal view returns (uint256) {
        Double memory supplyIndex = Double({ mantissa: supplyState.index });
        Double memory supplierIndex = Double({
            mantissa: rewardsDistributor.rewardTokenSupplierIndex(vToken, supplier)
        });
//      @audit
//        if (supplierIndex.mantissa == 0 && supplyIndex.mantissa  >= rewardsDistributor.rewardTokenInitialIndex()) {
        if (supplierIndex.mantissa == 0 && supplyIndex.mantissa > 0) {
            // Covers the case where users supplied tokens before the market's supply state index was set
            supplierIndex.mantissa = rewardsDistributor.rewardTokenInitialIndex();
        }
        Double memory deltaIndex = sub_(supplyIndex, supplierIndex);
        uint256 supplierTokens = VToken(vToken).balanceOf(supplier);
        uint256 supplierDelta = mul_(supplierTokens, deltaIndex);
        return supplierDelta;
    }

contracts/Lens/PoolLens.sol#L516

Inside rewardsDistributor original functions written likes this

    function _distributeSupplierRewardToken(address vToken, address supplier) internal {
...
        if (supplierIndex == 0 && supplyIndex >= rewardTokenInitialIndex) {
            // Covers the case where users supplied tokens before the market's supply state index was set.
            // Rewards the user with REWARD TOKEN accrued from the start of when supplier rewards were first
            // set for the market.
            supplierIndex = rewardTokenInitialIndex;
        }
...
    }

contracts/Rewards/RewardsDistributor.sol#L340

    function _distributeBorrowerRewardToken(
        address vToken,
        address borrower,
        Exp memory marketBorrowIndex
    ) internal {
...
        if (borrowerIndex == 0 && borrowIndex >= rewardTokenInitialIndex) {
            // Covers the case where users borrowed tokens before the market's borrow state index was set.
            // Rewards the user with REWARD TOKEN accrued from the start of when borrower rewards were first
            // set for the market.
            borrowerIndex = rewardTokenInitialIndex;
        }
...
}

Rewards/RewardsDistributor.sol#L374

Tools Used

Recommended Mitigation Steps

    function calculateSupplierReward(
        address vToken,
        RewardsDistributor rewardsDistributor,
        address supplier,
        RewardTokenState memory supplyState
    ) internal view returns (uint256) {
        Double memory supplyIndex = Double({ mantissa: supplyState.index });
        Double memory supplierIndex = Double({
            mantissa: rewardsDistributor.rewardTokenSupplierIndex(vToken, supplier)
        });
-        if (supplierIndex.mantissa == 0 && supplyIndex.mantissa > 0) {
+        if (supplierIndex.mantissa == 0 && supplyIndex.mantissa  >= rewardsDistributor.rewardTokenInitialIndex()) {
            // Covers the case where users supplied tokens before the market's supply state index was set
            supplierIndex.mantissa = rewardsDistributor.rewardTokenInitialIndex();
        }
        Double memory deltaIndex = sub_(supplyIndex, supplierIndex);
        uint256 supplierTokens = VToken(vToken).balanceOf(supplier);
        uint256 supplierDelta = mul_(supplierTokens, deltaIndex);
        return supplierDelta;
    }

    function calculateBorrowerReward(
        address vToken,
        RewardsDistributor rewardsDistributor,
        address borrower,
        RewardTokenState memory borrowState,
        Exp memory marketBorrowIndex
    ) internal view returns (uint256) {
        Double memory borrowIndex = Double({ mantissa: borrowState.index });
        Double memory borrowerIndex = Double({
            mantissa: rewardsDistributor.rewardTokenBorrowerIndex(vToken, borrower)
        });
-        if (borrowerIndex.mantissa == 0 && borrowIndex.mantissa > 0) {
+        if (borrowerIndex.mantissa == 0 && borrowIndex.mantissa >= rewardsDistributor.rewardTokenInitialIndex()) {
            // Covers the case where users borrowed tokens before the market's borrow state index was set
            borrowerIndex.mantissa = rewardsDistributor.rewardTokenInitialIndex();
        }
        Double memory deltaIndex = sub_(borrowIndex, borrowerIndex);
        uint256 borrowerAmount = div_(VToken(vToken).borrowBalanceStored(borrower), marketBorrowIndex);
        uint256 borrowerDelta = mul_(borrowerAmount, deltaIndex);
        return borrowerDelta;
    }

Assessed type

Invalid Validation

QA Report

See the markdown file with the details of this report here.

QA Report

See the markdown file with the details of this report here.

Gas Optimizations

See the markdown file with the details of this report here.

The poolRegistry variable is not initialized, so updateAssetsState function will never work

Lines of code

https://github.com/code-423n4/2023-05-venus/blob/9853f6f4fe906b635e214b22de9f627c6a17ba5b/contracts/RiskFund/ReserveHelpers.sol#L20

Vulnerability details

Summary

The poolRegistry variable is not initialized anywhere in the ReserveHelpers.sol contract, and there is no way to set it, which means that it will always have a default value of 0x0. The updateAssetsState() function relies on the poolRegistry variable to be set, as the function has a requirement: require(poolRegistry != address(0), "ReserveHelpers: Pool Registry address is not set") and if it is not set, the function will always revert.

Impact

As the contract is upgradeable, the impact is not high, because the owner can upgrade, and add the setter for the poolRegistry variable.

Code Snippet

contract ReserveHelpers {
  [..]
    address internal poolRegistry;
  [..]

    function updateAssetsState(address comptroller, address asset) public virtual {
        require(ComptrollerInterface(comptroller).isComptroller(), "ReserveHelpers: Comptroller address invalid");
        require(asset != address(0), "ReserveHelpers: Asset address invalid");
        require(poolRegistry != address(0), "ReserveHelpers: Pool Registry address is not set");
        require(
            PoolRegistryInterface(poolRegistry).getVTokenForAsset(comptroller, asset) != address(0),
            "ReserveHelpers: The pool doesn't support the asset"
        );

        uint256 currentBalance = IERC20Upgradeable(asset).balanceOf(address(this));
        uint256 assetReserve = assetsReserves[asset];
        if (currentBalance > assetReserve) {
            uint256 balanceDifference;
            unchecked {
                balanceDifference = currentBalance - assetReserve;
            }
            assetsReserves[asset] += balanceDifference;
            poolsAssetsReserves[comptroller][asset] += balanceDifference;
            emit AssetsReservesUpdated(comptroller, asset, balanceDifference);
        }
    }
}

Tools Used

Manual review

Recommended Mitigation Steps

To fix this vulnerability, the poolRegistry variable should be initialized properly, either during contract deployment or through a setter function. Additionally, appropriate checks should be put in place to ensure that the poolRegistry variable is always set before calling the updateAssetsState() function to prevent any potential vulnerabilities.

Assessed type

Other

QA Report

See the markdown file with the details of this report here.

Users can borrow the borrowCap amount, but they should borrow less than that and not equal

Lines of code

https://github.com/code-423n4/2023-05-venus/blob/9853f6f4fe906b635e214b22de9f627c6a17ba5b/contracts/Comptroller.sol#L354

Vulnerability details

Impact

Detailed description of the impact of this finding.
Users can borrow up to the borrowCap amount, but they should borrow less than that.

Proof of Concept

Provide direct links to all referenced code in GitHub. Add screenshots, logs, or any other relevant proof that illustrates the concept.
According to the docs from the code function Borrowing that brings total borrows to borrow cap will revert

    /**
     * @notice Set the given borrow caps for the given vToken markets. Borrowing that brings total borrows to or above borrow cap will revert.
...

contracts/Comptroller.sol#L839

this means that whevener user should not be able to hit borrowCap and revert if nextTotalBorrows == borrowCap but it doesn't

        if (borrowCap != type(uint256).max) {
            uint256 totalBorrows = VToken(vToken).totalBorrows();
            uint256 nextTotalBorrows = totalBorrows + borrowAmount;
            //          @audit should be, users can borrow more than allowed
            //            if (nextTotalBorrows >= borrowCap) {
            if (nextTotalBorrows > borrowCap) {
                revert BorrowCapExceeded(vToken, borrowCap);
            }
        }

contracts/Comptroller.sol#L354

Tools Used

Recommended Mitigation Steps

Same issue with supplyCap or change documentation

        if (borrowCap != type(uint256).max) {
            uint256 totalBorrows = VToken(vToken).totalBorrows();
            uint256 nextTotalBorrows = totalBorrows + borrowAmount;
-            if (nextTotalBorrows > borrowCap) {
+            if (nextTotalBorrows >= borrowCap) {
                revert BorrowCapExceeded(vToken, borrowCap);
            }
        }

        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) {
+            if (nextTotalSupply >= supplyCap) {
                revert SupplyCapExceeded(vToken, supplyCap);
            }
        }

Assessed type

Invalid Validation

Borrower can't repay but can have their collateral seized

Lines of code

https://github.com/code-423n4/2023-05-venus/blob/main/contracts/Comptroller.sol#L578-L626
https://github.com/code-423n4/2023-05-venus/blob/main/contracts/VToken.sol#L390-L429
https://github.com/code-423n4/2023-05-venus/blob/main/contracts/VToken.sol#L1097-L1136

Vulnerability details

Impact

Borrower can be prevented from repaying and while in this state can have their collateral seized. This severely disadvantages the Borrower guaranteeing loss of their collateral with no possibility to repay.

Proof of Concept

A major logical invariant of DeFi Lending/Borrowing systems is that the system should never be able to enter a state where a Borrower can't repay, but can be liquidated/have their collateral seized.

Venus tries to preserve this invariant as VToken._liquidateBorrowFresh() calls VToken._repayBorrowFresh() which calls Comptroller.preRepayHook() which reverts liquidation if Action.REPAY is paused.

However there are 3 transitions into this invariant-breaking state possible through:

1) Comptroller.healAccount() -> VToken.seize()
2) Comptroller.healAccount() -> VToken.healBorrow()
3) Calling VToken.seize() directly

Each of these paths take all of the Borrower's collateral even when the Borrower is prevented from repaying because Action.REPAY is paused on the market.

PoC add to healAccountTest.ts under describe("liquidation incentive * debt > collateral":

// @audit PoC - Borrower can't repay but can have their collateral seized via
//              Comptroller.healAccount() -> VToken.seize()
it("borrower can't repay then seize collateral via Comptroller.healAccount() -> VToken.seize()", async () => {
  // pause REPAY action on OMG & ZRX markets, borrowers now can't repay
  await comptroller.setActionsPaused([OMG.address, ZRX.address], [3], true);
  expect(await comptroller.actionPaused(OMG.address, 3)).to.equal(true);
  expect(await comptroller.actionPaused(ZRX.address, 3)).to.equal(true);

  // but they can still have their collateral seized
  // via Comptroller.healAccount() -> VToken.seize()
  const omgToSeize = parseUnits("4", 18);
  const zrxToSeize = parseUnits("7", 18);
  await comptroller.connect(liquidator).healAccount(user.address);
  expect(OMG.seize).to.have.been.calledWith(liquidator.address, user.address, omgToSeize);
  expect(ZRX.seize).to.have.been.calledWith(liquidator.address, user.address, zrxToSeize);
  expect(BAT.seize).to.have.not.been.called;
});

// @audit PoC - Borrower can't repay but can have their collateral seized via
//              Comptroller.healAccount() -> VToken.healBorrow()
it("borrower can't repay then seize collateral via Comptroller.healAccount() -> VToken.healBorrow()", async () => {
  // pause REPAY action on ZRX & BAT markets, borrowers now can't repay
  await comptroller.setActionsPaused([ZRX.address, BAT.address], [3], true);
  expect(await comptroller.actionPaused(ZRX.address, 3)).to.equal(true);
  expect(await comptroller.actionPaused(BAT.address, 3)).to.equal(true);

  // but they can still have their collateral seized
  // via Comptroller.healAccount() -> VToken.healBorrow()
  // total borrows = 600 + 400
  // borrows to repay = (4 + 7) / 1.1 = 10
  // percentage = 10 / 1000 = 1%
  const zrxToRepay = parseUnits("6", 18);
  const batToRepay = parseUnits("4", 18);
  await comptroller.connect(liquidator).healAccount(user.address);
  expect(OMG.healBorrow).to.have.not.been.called;
  expect(ZRX.healBorrow).to.have.been.calledWith(liquidator.address, user.address, zrxToRepay);
  expect(BAT.healBorrow).to.have.been.calledWith(liquidator.address, user.address, batToRepay);
});

// @audit PoC - Borrower can't repay but can have their collateral seized via 
//              calling VToken.seize() directly
it("borrower can't repay then seize collateral via VToken.seize()", async () => {
  // pause REPAY action on OMG & ZRX markets, borrowers now can't repay
  await comptroller.setActionsPaused([OMG.address, ZRX.address], [3], true);
  expect(await comptroller.actionPaused(OMG.address, 3)).to.equal(true);
  expect(await comptroller.actionPaused(ZRX.address, 3)).to.equal(true);

  // but they can still have their collateral seized
  // via VToken.seize()
  const omgToSeize = parseUnits("4", 18);
  const zrxToSeize = parseUnits("7", 18);
  await OMG.connect(liquidator).seize(liquidator.address, user.address, omgToSeize);
  await ZRX.connect(liquidator).seize(liquidator.address, user.address, zrxToSeize);
  expect(OMG.seize).to.have.been.calledWith(liquidator.address, user.address, omgToSeize);
  expect(ZRX.seize).to.have.been.calledWith(liquidator.address, user.address, zrxToSeize);
  expect(BAT.seize).to.have.not.been.called;
});

Run with: yarn test --grep "borrower can't repay"

Tools Used

Weaponized Autism (I examined every single H/M finding in every lending/borrowing contest in the history of audit contests. Kinda crazy right?)

Recommended Mitigation Steps

In order to be fair to Borrowers, they should not be able to have their collateral seized if they are in a state of being prevented from repaying.

Further consideration: assuming a fix is implemented that prevents Borrowers from having their collateral seized while repayments are paused, a further issue must be considered. While repayments are paused Borrowers may become liable to liquidation/collateral seized, which could occur immediately upon repayments being unpaused, done via liquidation bots that would front-run any human Borrower attempting to repay.

To be fair to human Borrowers after repayments are unpaused there should be a grace period (perhaps equal to the length of time repayments were paused) during which Borrowers can't be liquidated or have their collateral seized, to allow human Borrowers the opportunity to repay. This is essential to be fair to Borrowers who became subject to liquidation through no fault of their own, but only due to being unable to repay while repayments were paused. Please see my other issue "Borrower immediately liquidated after repayments resume" for more discussion on this topic.

Assessed type

Other

QA Report

See the markdown file with the details of this report here.

QA Report

See the markdown file with the details of this report here.

VToken.mint(), mintBehalf() & redeem() expose users to unlimited slippage

Lines of code

https://github.com/code-423n4/2023-05-venus/blob/main/contracts/VToken.sol#L180-L183
https://github.com/code-423n4/2023-05-venus/blob/main/contracts/VToken.sol#L195-L200
https://github.com/code-423n4/2023-05-venus/blob/main/contracts/VToken.sol#L743-L791
https://github.com/code-423n4/2023-05-venus/blob/main/contracts/VToken.sol#L1463-L1482

Vulnerability details

Impact

VToken.mint() & mintBehalf() are functionally swaps by another name as they allow users to swap underlying assets for protocol tokens, but they don't allow users to specify a minimum amount of protocol tokens to be received or an expiration deadline for the swap. This is a known Slippage vulnerability class Minting Exposes Users To Unlimited Slippage which was judged a High Finding in C4 Vader contest.

Proof of Concept

The amount of output protocol tokens is calculated using an exchange rate calculated in _exchangeRateStored() which uses on-chain parameters [totalCash, totalSupply, totalBorrows, badDebt & totalReserves] that can be manipulated on-chain by other market participants.

However the user can’t specify the minimum output amount of protocol tokens that they would accept nor a deadline by which the transaction should be completed. This is equivalent to a swap with No Slippage Parameter and No Expiration Deadline.

When the user tries to mint() the pool parameters may look attractive but an attacker can sandwich attack the user's transaction, manipulating the pool parameters to dramatically reduce the amount of output protocol tokens the user was expecting to receive, as the user is exposed to the swap with unlimited slippage.

The same is true for redeem() which runs the same process just in the opposite direction, can result in user receiving fewer underlying assets than they expected.

May also impact _seize().

Tools Used

Weaponized Autism; I read through every H/M Slippage vulnerability in the history of smart contract audit contests, kinda crazy right?

Recommended Mitigation Steps

In VToken.mint() & mintBehalf() allow users to specify a minimum amount of protocol tokens to be received and optionally also an expiration deadline parameter by which the transaction must be completed.

In VToken._mintFresh() revert if the amount of protocol tokens received < minimum or if block.timestamp > expiration deadline.

In VToken.redeem() allow user to specify minimum amount of underlying asset to receive and optionally an expiration deadline, pass these to _redeemFresh() and make necessary changes in _redeemFresh().

Assessed type

MEV

Inconsistent scaling of USD in bad debt in the project.

Lines of code

https://github.com/code-423n4/2023-05-venus/blob/9853f6f4fe906b635e214b22de9f627c6a17ba5b/contracts/Shortfall/Shortfall.sol#L393

Vulnerability details

Impact

Detailed description of the impact of this finding.
Inconsistent scaling of USD in bad debt in the project.

Proof of Concept

Provide direct links to all referenced code in GitHub. Add screenshots, logs, or any other relevant proof that illustrates the concept.
usdvalue is being calculated differently in project. This is how its calculated for auction, As you can see its being scaled down by / 1e18. Its the only place in project where getUnderlyingPrice * tokenAmount is being scaled down.

        for (uint256 i; i < marketsCount; ++i) {
            uint256 marketBadDebt = vTokens[i].badDebt();

            priceOracle.updatePrice(address(vTokens[i]));
            uint256 usdValue = (priceOracle.getUnderlyingPrice(address(vTokens[i])) * marketBadDebt) / 1e18;

            poolBadDebt = poolBadDebt + usdValue;
            auction.markets[i] = vTokens[i];
            auction.marketDebt[vTokens[i]] = marketBadDebt;
            marketsDebt[i] = marketBadDebt;
        }

contracts/Shortfall/Shortfall.sol#L393

This is how badDebtUsd is being calculated inside poollens

        for (uint256 i; i < markets.length; ++i) {
            BadDebt memory badDebt;
            badDebt.vTokenAddress = address(markets[i]);
            badDebt.badDebtUsd =
                VToken(address(markets[i])).badDebt() *
                priceOracle.getUnderlyingPrice(address(markets[i]));
            badDebtSummary.badDebts[i] = badDebt;
            totalBadDebtUsd = totalBadDebtUsd + badDebt.badDebtUsd;
        }

contracts/Lens/PoolLens.sol#L268

Tools Used

Manual

Recommended Mitigation Steps

As I understood, scaling down should be removed inside shortfall due the only place in the project where getUnderlyingPrice * tokenAmount is being scaled down

        for (uint256 i; i < marketsCount; ++i) {
            uint256 marketBadDebt = vTokens[i].badDebt();

            priceOracle.updatePrice(address(vTokens[i]));
-            uint256 usdValue = (priceOracle.getUnderlyingPrice(address(vTokens[i])) * marketBadDebt) / 1e18;
+            uint256 usdValue = (priceOracle.getUnderlyingPrice(address(vTokens[i])) * marketBadDebt);

            poolBadDebt = poolBadDebt + usdValue;
            auction.markets[i] = vTokens[i];
            auction.marketDebt[vTokens[i]] = marketBadDebt;
            marketsDebt[i] = marketBadDebt;
        }

Assessed type

Math

QA Report

See the markdown file with the details of this report here.

QA Report

See the markdown file with the details of this report here.

TEST ISSUE

Lines of code

https://github.com/code4rena-dev/code4rena.com/blob/c00364df165549efa4fdd6ee657ecc576d2cee44/.gitignore#L1

Vulnerability details

Impact

Detailed description of the impact of this finding.

Proof of Concept

Provide direct links to all referenced code in GitHub. Add screenshots, logs, or any other relevant proof that illustrates the concept.

Tools Used

Recommended Mitigation Steps

Gas Optimizations

See the markdown file with the details of this report here.

updateAssetsState() function has reentrancy vulnerability

Lines of code

https://github.com/code-423n4/2023-05-venus/blob/main/contracts/RiskFund/ReserveHelpers.sol#L59

Vulnerability details

Impact

Reentrancy vulnerability cause DoS

Proof of Concept

https://github.com/code-423n4/2023-05-venus/blob/main/contracts/RiskFund/ReserveHelpers.sol#L59

The asset parameters is controlled by the attacker，attacker can use a fake contract and cause reentrancy vulnerability.

Tools Used

sublimed

Recommended Mitigation Steps

Add modifier Noreentrancy

Assessed type

DoS

A malicious Actor can use healAccount() function in comptroller.sol to seize users tokens

Lines of code

https://github.com/code-423n4/2023-05-venus/blob/8be784ed9752b80e6f1b8b781e2e6251748d0d7e/contracts/Comptroller.sol#L578-L626
https://github.com/code-423n4/2023-05-venus/blob/8be784ed9752b80e6f1b8b781e2e6251748d0d7e/contracts/VToken.sol#L473
https://github.com/code-423n4/2023-05-venus/blob/8be784ed9752b80e6f1b8b781e2e6251748d0d7e/contracts/VToken.sol#L1097-L1136

Vulnerability details

Impact

The healAccount() function in comptroller.sol can be used by a malicious actor to steal tokens from users as anyone who calls the healAccount() function automatically becomes the liquidator.

This is because of this line address liquidator = msg.sender;

Now this check here if (tokens != 0) { market.seize(liquidator, user, tokens); }
checks to ensure there are tokens owned by the user(victim) and seizes them.

checking the seize() function it calls the _seize() function and the _seize() function decreases the token balance of the user(victim) and increases the token balance of the attacker(liquidator).

that takes place here:

        /* We write the calculated values into storage */
        ***
        ***
        accountTokens[borrower] = accountTokens[borrower] - seizeTokens;
        accountTokens[liquidator] = accountTokens[liquidator] + liquidatorSeizeTokens;

Proof of Concept

There's no access control in the healBorrow() function in comptroller and it's external so anyone can call it and play the role of a liquidator because the function assumes everyone who interacts with it is the liquidator.

Tools Used

Manual Review

Recommended Mitigation Steps

Implement Access control on the healAccount() function

Assessed type

Access Control

Repay and redeem action, if paused, will expose borrowing users to unnecessary extra debt and risk

Lines of code

https://github.com/code-423n4/2023-05-venus/blob/main/contracts/Comptroller.sol#L391
https://github.com/code-423n4/2023-05-venus/blob/main/contracts/Comptroller.sol#L297

Vulnerability details

Summary

In the Comptroller contract, the admin/governance is allowed to pause any actions interacting with vTokens. We argue that, even for a non-malicious admin, pausing these actions can introduce problems.

Vulnerability details

There are a number of actions that can be paused by the admin, among those includes REPAY, allowing users to repay their own debt (or even on behalf of others), and REDEEM, allowing users to withdraw their own funds.

function preRepayHook(address vToken, address borrower) external override {
    _checkActionPauseState(vToken, Action.REPAY);
    //...
}

function preRedeemHook(
    address vToken,
    address redeemer,
    uint256 redeemTokens
) external override {
    _checkActionPauseState(vToken, Action.REDEEM);
    //...
}

We argue that, even for a non-malicious admin/governance, pausing these actions will be problematic for the user:

• If REPAY is paused, any borrowing users will not be able to repay. Since interest still accrues during the paused period, they end up having to pay more debt than they would have.
  • Since the user accrues more debt, this will also open up increased possibilities for liquidation. This is a more urgent risk, since this is directly related to fund loss.
• If REDEEM is paused, users will not be able to withdraw funds. This is more of a centralization risk, since it effectively allows governance to lock users' funds at will, and will affect users' trust in the protocol.
• If any of these actions are paused but not SEIZE or LIQUIDATION alongside that, users' funds are put at direct risk of liquidation, and they have no way to manage said risk.

Impact

• For the REPAY issue, users end up accruing more debt than they should have, thus ends up having to repay more.
• For the other actions, the main impacts are those related to centralization (e.g. users' trust), however will also expose funds to unnecessary risks.

Tools Used

Manual review

Recommended Mitigation Steps

Actions that allow users to withdraw funds (REPAY, REDEEM) should not be pausable, even by admins.

Assessed type

Other

QA Report

See the markdown file with the details of this report here.

no check against incorrect price for assets in healAccount() function

Lines of code

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

Vulnerability details

Impact

there's no check to ensure price used here is not an incorrect price.

there should be a check like this

        if (oracle.getUnderlyingPrice(vToken) == 0) {
            revert PriceError(address(vToken));
        }

to ensure the oracle doesn't return incorrect price for some assets.

before the oracle.updatePrice(vToken) is done.

Proof of Concept

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

Tools Used

Manual Review

Recommended Mitigation Steps

implement a check against oracles using incorrect prices for the healAccount() function and ensure a revert whenever incorrect prices are used for assets.
something like this

        if (oracle.getUnderlyingPrice(vToken) == 0) {
            revert PriceError(address(vToken));
        }

Assessed type

Oracle

QA Report

See the markdown file with the details of this report here.

QA Report

See the markdown file with the details of this report here.

Agreements & Disclosures

Agreements

If you are a C4 Certified Contributor by commenting or interacting with this repo prior to public release of the contest report, you agree that you have read the Certified Warden docs and agree to be bound by:

• C4 Certified Contributor Terms and Conditions,
• C4 Code of Professional Conduct, and
• the disclosure guidelines below.

To signal your agreement to these terms, add a 👍 emoji to this issue.

Code4rena staff reserves the right to disqualify anyone from this role and similar future opportunities who is unable to participate within the above guidelines.

Disclosures

Sponsors may elect to add team members and contractors to assist in sponsor review and triage. All sponsor representatives added to the repo should comment on this issue to identify themselves.

To ensure contest integrity, the following potential conflicts of interest should also be disclosed with a comment in this issue:

1. any sponsor staff or sponsor contractors who are also participating as wardens
2. any wardens hired to assist with sponsor review (and thus presenting sponsor viewpoint on findings)
3. any wardens who have a relationship with a judge that would typically fall in the category of potential conflict of interest (family, employer, business partner, etc)
4. any other case where someone might reasonably infer a possible conflict of interest.

QA Report

See the markdown file with the details of this report here.

QA Report

See the markdown file with the details of this report here.

QA Report

See the markdown file with the details of this report here.

QA Report

See the markdown file with the details of this report here.

QA Report

See the markdown file with the details of this report here.

Borrow rate calculation can cause VToken.accrueInterest() to revert, DoSing all major functionality

Lines of code

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

Vulnerability details

Impact

Borrow rates are calculated dynamically and VToken.accrueInterest() reverts if the calculated rate is greater than a hard-coded maximum. As accrueInterest() is called by most VToken functions, this state causes a major DoS.

Proof of Concept

VToken hard-codes the maximum borrow rate and accrueInterest() reverts if the dynamically calculated rate is greater than the hard-coded value.

The actual calculation is dynamic [1, 2] and takes no notice of the hard-coded cap, so it is very possible that this state will manifest, causing a major DoS due to most VToken functions calling accrueInterest() and accrueInterest() reverting.

Tools Used

Manual review

Recommended Mitigation Steps

Change VToken.accrueInterest() to not revert in this case but simply to set borrowRateMantissa = borrowRateMaxMantissa if the dynamically calculated value would be greater than the hard-coded max. This would:

1. allow execution to continue operating with the system-allowed maximum borrow rate, allowing all functionality that depends upon accrueInterest() to continue as normal,

2. allow borrowRateMantissa to be naturally set to the dynamically calculated rate as soon as that rate becomes less than the hard-coded max.

Assessed type

DoS

QA Report

See the markdown file with the details of this report here.

QA Report

See the markdown file with the details of this report here.

No access control or permission mechanism for who can call the `deployVTokenProxy` function.

Lines of code

https://github.com/code-423n4/2023-05-venus/blob/8be784ed9752b80e6f1b8b781e2e6251748d0d7e/contracts/Factories/VTokenProxyFactory.sol#L11-L50

Vulnerability details

Impact

There is no access control or permission mechanism for who can call the deployVTokenProxy function. Anyone can call this function and deploy new VToken proxies, which could lead to excessive gas consumption or other attacks. It would be safer to add some sort of access control mechanism to restrict who can call this function.

Proof of Concept

To add access control and permission mechanism to restrict who can call the deployVTokenProxy function, I modify the code to include a modifier that checks whether the caller is authorized to call the function. Here's an example:

// SPDX-License-Identifier: BSD-3-Clause
pragma solidity 0.8.13;

import "@openzeppelin/contracts/proxy/beacon/BeaconProxy.sol";
import "../VToken.sol";
import "@venusprotocol/governance-contracts/contracts/Governance/AccessControlManager.sol";
import "../VTokenInterfaces.sol";

contract VTokenProxyFactory {
    struct VTokenArgs {
        address underlying_;
        ComptrollerInterface comptroller_;
        InterestRateModel interestRateModel_;
        uint256 initialExchangeRateMantissa_;
        string name_;
        string symbol_;
        uint8 decimals_;
        address admin_;
        AccessControlManager accessControlManager_;
        VTokenInterface.RiskManagementInit riskManagement;
        address beaconAddress;
        uint256 reserveFactor;
    }

    event VTokenProxyDeployed(VTokenArgs args);

    address public admin;

    constructor(address _admin) {
        admin = _admin;
    }

    modifier onlyAdmin() {
        require(msg.sender == admin, "VTokenProxyFactory: caller is not the admin");
        _;
    }

    function deployVTokenProxy(VTokenArgs memory input) external onlyAdmin returns (VToken) {
        BeaconProxy proxy = new BeaconProxy(
            input.beaconAddress,
            abi.encodeWithSelector(
                VToken.initialize.selector,
                input.underlying_,
                input.comptroller_,
                input.interestRateModel_,
                input.initialExchangeRateMantissa_,
                input.name_,
                input.symbol_,
                input.decimals_,
                input.admin_,
                input.accessControlManager_,
                input.riskManagement,
                input.reserveFactor
            )
        );

        emit VTokenProxyDeployed(input);

        return VToken(address(proxy));
    }
}

In my modified code, I added an onlyAdmin modifier that checks whether the caller is the admin. The admin is set during contract deployment and cannot be changed afterward. Only the admin can call the deployVTokenProxy function. This ensures that only authorized parties can create new VToken proxies, reducing the risk of excessive gas consumption or other attacks.

Tools Used

VSCode

Recommended Mitigation Steps

a. Implement access control: Add an access control mechanism to restrict who can call the deployVTokenProxy function. This can be done using a role-based access control system, such as the OpenZeppelin Access Control library.

b. Use a whitelist: Create a whitelist of trusted addresses that are allowed to deploy new VToken proxies. Only allow the deployVTokenProxy function to be called by addresses on the whitelist.

c. Implement a circuit breaker: Add a circuit breaker mechanism to pause the deployVTokenProxy function in case of an emergency or unexpected behavior.

By implementing these mitigation steps, the VTokenProxyFactory contract can be made more secure and reduce the risk of unauthorized deployment of VToken proxies.

Assessed type

Access Control

QA Report

See the markdown file with the details of this report here.

RiskFund/ReserveHelpers.sol has not constructor and the poolRegistry is undefined

Lines of code

https://github.com/code-423n4/2023-05-venus/blob/main/contracts/RiskFund/ReserveHelpers.sol#L20
https://github.com/code-423n4/2023-05-venus/blob/main/contracts/RiskFund/ReserveHelpers.sol#L53

Vulnerability details

Impact

Cause the updateAssetsState() function useless.

Proof of Concept

https://github.com/code-423n4/2023-05-venus/blob/main/contracts/RiskFund/ReserveHelpers.sol#L20

We can see the poolRegistry is undefined, and not find the constructor

address internal poolRegistry;

But in https://github.com/code-423n4/2023-05-venus/blob/main/contracts/RiskFund/ReserveHelpers.sol#L53

The updateAssetsState() function require the poolRegistry != address(0)

function updateAssetsState(address comptroller, address asset) public virtual {
        require(ComptrollerInterface(comptroller).isComptroller(), "ReserveHelpers: Comptroller address invalid");
        require(asset != address(0), "ReserveHelpers: Asset address invalid");
        require(poolRegistry != address(0), "ReserveHelpers: Pool Registry address is not set");
        require(
            PoolRegistryInterface(poolRegistry).getVTokenForAsset(comptroller, asset) != address(0),
            "ReserveHelpers: The pool doesn't support the asset"
        );

So updateAssetsState() function is useless.

In RiskFund/ProtocolShareReserve.sol, if owner does not call setPoolRegistry() ,and then user call updateAssetsState() is wrong.

Tools Used

sublimed

Recommended Mitigation Steps

Add constructor and init poolRegistry

Assessed type

Error

WhitePaperInterestRateModel.sol Inflates Interest Rate

Lines of code

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

Vulnerability details

The interest model created by WhitePaperInterestRateModel.sol incorrectly inflates the intended interest rate due to an incorrect hardcoding of the blocksPerYear variable. This will result in borrowers paying higher than intended interest, as this variable is used to calculate a vToken’s borrow rate.

BNB and Ethereum have different block rates, with BNB producing a block every 3 seconds and Ethereum’s proof of work variation producing a block every 13-15 seconds.

WhitePaperInterestRateModel.sol hardcodes the blocksPerYear variable to Ethereum’s block cadence instead of BNB’s. The value is set to 2_102_400 blocks per year instead of the needed 10_512_000 blocks per year.

This error will result in borrowers paying higher than intended interest rates, as the calculated baseRatePerBlock will be much larger than intended when using this contract as a vToken's interest rate.

// WhitePaperInterestRateModel's constructor 
constructor(uint256 baseRatePerYear, uint256 multiplierPerYear) {
        baseRatePerBlock = baseRatePerYear / blocksPerYear; // will be higher than intended
        multiplierPerBlock = multiplierPerYear / blocksPerYear; // also will be higher than intended

        emit NewInterestParams(baseRatePerBlock, multiplierPerBlock);
    }

Remediations to Consider:

• Changing the hardcoding of blocksPerYear to the proper value of 10_512_000

Assessed type

Other

Gas Optimizations

See the markdown file with the details of this report here.

QA Report

See the markdown file with the details of this report here.