Venus Protocol Isolated Pools - YoungWolves's results

[L-01] baseRatePerYear needs to be calculated differently for Jump- and WhitePaperInterest- Rate Models

Files:

PoolRegistry.sol L272 L280

BaseJumpRateModelV2.sol L23

WhitePaperInterestRateModel.sol L17

Description:

blocksPerYear is used as a constant in BaseJumpRateModelV2.sol and WhitePaperInterestRateModel.sol

While Venus has been forked from compound, they are deloyed on different networks BSC an Ethereum respectively. The difference in average block time is about 4x (3s for BSC, 12s for Ethereum).

In BaseJumpRateModelV2.sol the value of blocksPerYear has been adjusted for the network change to be equal to 10512000, in WhitePaperInterestRateModel.sol the value remains the same as in compound's original: 2102400

To confirm that this is indeed a mistake, here is a link to the current Venus github, where this value is calculated based on block time.

Taking it into consideration, when calling PoolRegistry#addMarket() the baseRatePerYear would have to be calculated differently, taking into account different values as mentioned above, to achieve the same rates.

[L-02] Difference between blocksPerYear hardcoded contant value in Jump- and WhitePaperInterest- Rate Models could lead to DOS if not spotted

Files:

PoolRegistry.sol L272 L280

BaseJumpRateModelV2.sol L23

WhitePaperInterestRateModel.sol L17 L56

VToken.sol L 158, 168, 181, 198, 214, 227, 242, 256, 271, 333, 346, 357, 372, 666, L695-696

Description:

blocksPerYear is used as a constant in BaseJumpRateModelV2.sol and WhitePaperInterestRateModel.sol

While Venus has been forked from compound, they are deloyed on different networks BSC an Ethereum respectively. The difference in average block time is about 4x (3s for BSC, 12s for Ethereum).

In BaseJumpRateModelV2.sol the value of blocksPerYear has been adjusted for the network change to be equal to 10512000, in WhitePaperInterestRateModel.sol the value remains the same as in compound's original: 2102400

To confirm that this is indeed a mistake, here is a link to the current Venus github, where this value is calculated based on block time:

If unnoticed, it could cause a market-wide DOS 5 times faster than expected:

In VToken.sol 14 different functions include accrueInterest() , which in turn checks the result of InterestRateModel.getBorrowRate() against the borrowRateMaxMantissa (L695-696). In WhitePaperInterestRateModel.sol getBorrowRate() returns ((ur * multiplierPerBlock) / BASE) + baseRatePerBlock where both multiplierPerBlock and baseRatePerBlock are dependant on blocksPerYear.

[NC-01] Extract duplicated code to a function

File:

ReserveHelpers.sol L39-40 L50-51

Such as:

modifier checkIsValidAddress private {
  require(ComptrollerInterface(comptroller).isComptroller(), "ReserveHelpers: Comptroller address invalid");
  require(asset != address(0), "ReserveHelpers: Asset address invalid");
  _;
}

[NC-02] Redundant Openzeppelin's Ownable2StepUpgradeable.sol import

Files:

VToken.sol L4 PoolRegistry.sol L4 RewardDistributor.sol L4 RiskFund.sol L4 Shortfall.sol L4

Description:

In all above mentioned contracts the following line: import @openzeppelin/contracts-upgradeable/access/Ownable2StepUpgradeable.sol; is redundant as all of them include: import @venusprotocol/governance-contracts/contracts/Governance/AccessControlledV8.sol;, which itself imports Ownable2StepUpgradeable.sol

[NC-03] Use inherited function to reduce duplicate code

File:

VToken.sol L1363-1364 PoolRegistry.sol L172-173 RewardDistributor.sol L119-120 RiskFund.sol L85-86 Shortfall.sol L141-142

AccessControlledV8.sol L34-37

Description:

In above mentioned contract the following code is used during initialization:

__Ownable2Step_init();
__AccessControlled_init_unchained(accessControlManager_);

It could be in all cases reduced to 1 line, as all the contract inherit from AccessControlledV8, which implements the following function:

function __AccessControlled_init(address accessControlManager_) internal onlyInitializing {
    __Ownable2Step_init();
    __AccessControlled_init_unchained(accessControlManager_);
}

[NC-04] Inherited variable shadowed. Consider changing name.

There is already a state variable called owner on OwnableUpgradeable contract. It would be convenient to rename that variable's name.

File:

VToken.sol L539 L548