Venus Protocol Isolated Pools - Franfran's results

Lines of code

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

Vulnerability details

Impact

For getting the interest rate related calculations in the VToken, two contracts may be used:

• BaseJumpRateModelV2
• WhitePaperInterestRateModel

Both are similar and serve the same purpose, but the BaseJumpRateModelV2 has a more unique feature, it supports non-linear borrow rates thanks to the setting of the kink variable.

For calculating the interest between two accrueInterest(), the delta of the last block at which the interest as been accrued last time is taken as a time unit. This is a reasonable choice considering that on the BNB Chain, the block time is consistent with about 3s per block.

In order to calculate the baseRatePerBlock and the multiplierPerBlock from the baseRatePerYear and multiplierPerYear respectively, we must use the blocksPerYear

The issue, is that this blocksPerYear is not calculated correctly. At the moment, the constant's value is 2102400. Let's make the calculation:

$blocksPerYear = \frac{year}{blocktime}$ $blocksPerYear = \frac{365*24*3600}{3}$ $blocksPerYear = 10512000$

This is roughly twice less than the constant that is currently in the code. So we will admit that the contract assumes a 6s block time.

Proof of Concept

Let's run a quick test to see what's the consequence on calculations for a 6s block time.

// SPDX-License-Identifier: UNLICENSED
pragma solidity ^0.8.13;

import "forge-std/Test.sol";

import "../contracts/WhitePaperInterestRateModel.sol";
import "../contracts/JumpRateModelV2.sol";

contract BlocksTest is Test {
    WhitePaperInterestRateModel wp;
    JumpRateModelV2 jr;

    uint256 baseRatePerYear = 1e18;
    uint256 multiplierPerYear = 1e18;

    function setUp() public {
        wp = new WhitePaperInterestRateModel(baseRatePerYear, multiplierPerYear);
    }

    function testRate() public {
        uint256 cash = 1e18;
        uint256 borrows = 2e17;
        uint256 reserves = 1e17;

        emit log_uint(wp.getBorrowRate(cash, borrows, reserves));
        emit log_uint(fixedGetBorrowRate(cash, borrows, reserves));
    }

    function fixedGetBorrowRate(
        uint256 cash,
        uint256 borrows,
        uint256 reserves
    ) public view returns (uint256) {
        uint256 ur = wp.utilizationRate(cash, borrows, reserves);
        /// @audit-ok rate{(bor/supply)/block} = (ur{bor/supply} * val{int/block}) + rate{APR/block}
        uint256 baseRatePerBlock = baseRatePerYear / 10512000;
        return ((ur * wp.multiplierPerBlock()) / 1e18) + baseRatePerBlock;
    }
}

output:

[⠑] Compiling...
[⠑] Compiling 1 files with 0.8.13
[⠊] Solc 0.8.13 finished in 1.93s
Compiler run successful

Running 1 test for test/Blocks.t.sol:BlocksTest
[PASS] testRate() (gas: 12587)
Logs:
  562128130620
  181610626815

Test result: ok. 1 passed; 0 failed; finished in 849.25µs

As you can see, the first one (current implementation) assuming a 6s block time compensate by returning a bigger borrow rate than intented.

Some quite bad consequences are:

• Potential freezing of the markets because of an overly high borrow rate
• Too much interests distributed to users

Tools Used

Manual inspection

Recommended Mitigation Steps

Replace blocksPerYear by 10512000 in the WhitePaperInterestRateModel contract

Assessed type

Math

[Low - 1] Oracle zero price check is not always done

Examples of when it is checked:

• https://github.com/code-423n4/2023-05-venus/blob/main/contracts/Comptroller.sol#L345-L347
• https://github.com/code-423n4/2023-05-venus/blob/main/contracts/Comptroller.sol#L755C10-L757
• https://github.com/code-423n4/2023-05-venus/blob/main/contracts/Comptroller.sol#L1369C1-L1372

And when it is not:

• https://github.com/code-423n4/2023-05-venus/blob/main/contracts/RiskFund/RiskFund.sol#L240C8-L242
• https://github.com/code-423n4/2023-05-venus/blob/main/contracts/Shortfall/Shortfall.sol#L393
• https://github.com/code-423n4/2023-05-venus/blob/main/contracts/Lens/PoolLens.sol#L266-L268
• https://github.com/code-423n4/2023-05-venus/blob/main/contracts/Lens/PoolLens.sol#L405-L409

Submitted as "Low" because the oracle is OOS.

[Low - 2] NO_ERROR check is not always handled

The NO_ERROR constant is returned to notify that the execution of the last call, even if didn't reverted, went smoothly. This should be handled.

• https://github.com/code-423n4/2023-05-venus/blob/main/contracts/VToken.sol#LL158C11-L158C11
• https://github.com/code-423n4/2023-05-venus/blob/main/contracts/VToken.sol#L168
• https://github.com/code-423n4/2023-05-venus/blob/main/contracts/VToken.sol#L181
• https://github.com/code-423n4/2023-05-venus/blob/main/contracts/VToken.sol#L198
• https://github.com/code-423n4/2023-05-venus/blob/main/contracts/VToken.sol#L214
• https://github.com/code-423n4/2023-05-venus/blob/main/contracts/VToken.sol#L227
• https://github.com/code-423n4/2023-05-venus/blob/main/contracts/VToken.sol#L242
• https://github.com/code-423n4/2023-05-venus/blob/main/contracts/VToken.sol#L256
• https://github.com/code-423n4/2023-05-venus/blob/main/contracts/VToken.sol#L271
• https://github.com/code-423n4/2023-05-venus/blob/main/contracts/VToken.sol#L333
• https://github.com/code-423n4/2023-05-venus/blob/main/contracts/VToken.sol#L346
• https://github.com/code-423n4/2023-05-venus/blob/main/contracts/VToken.sol#L357
• https://github.com/code-423n4/2023-05-venus/blob/main/contracts/VToken.sol#L372
• https://github.com/code-423n4/2023-05-venus/blob/main/contracts/VToken.sol#LL666C1-L666C1
• https://github.com/code-423n4/2023-05-venus/blob/main/contracts/VToken.sol#L996

[Low - 3] Wrong emitted event

In the healBorrow() function of the VToken contract, the HealBorrow() event is emitted at the end: https://github.com/code-423n4/2023-05-venus/blob/main/contracts/VToken.sol#L428 But the last element appears to be wrong. It emits repayAmount, but we probably need actualRepayAmount, in the case of using fee-on-transfer tokens.

[Low - 4] delete on a dynamic type does not zeroes out all the elements

In the Shortfall contract, when creating a new auction, we must empty the old values of the last one. This is done by cleaning the Auction struct with multiple tricks. When it comes to cleaning the array market field, the delete keyword is used: https://github.com/code-423n4/2023-05-venus/blob/main/contracts/Shortfall/Shortfall.sol#LL379C13-L379C13 But using the delete keyword on a dynamic type only resets its length to 0. Luckily, this doesn't seem to have any bad consequences but it could have got under different circumstances.

[NC - 1] Needless address check

In the _liquidateBorrowFresh() function of the VToken contract used to liquidate underwater accounts, there is a check to make sure that the liquidator isn't the borrower at the same time: https://github.com/code-423n4/2023-05-venus/blob/main/contracts/VToken.sol#LL1046C48-L1046C48 It seems like this check is useless because it is already done in the seize() function that is called later on.