Sign Up for a Code Audit Today and Get 3 Months of Skynet for Free

In the whole system, `NO_ERROR` corresponds to the default value of an `enum`. Hence, if a function is supposed to return an error but has bad implementation it will be harder to spot because the value returned will `NO_ERROR`.

Unsafe Error Handling

The `adminOrInitializing()` function requires that the `msg.sender` is the `admin` or `comptrollerImplementation`. However, in the linked codes the error messages always say that only the admin can call these functions.

Inaccurate Error Message

The functions which are never called internally within the contract should have `external` visibility for gas optimization.

Function Should Be Declared `external`

`ComptrollerLens` implements the interface `ComptrollerLensInterface`, but does not inherit from it.

Missing Inheritance

In the contract `Comptroller` the role `admin` has authority over the following functions:
- `_setPriceOracle()`
- `_setCloseFactor()`
- `_setCollateralFactor()`
- `_setLiquidationIncentive()`
- `_setLiquidatorContract()`
- `_supportMarket()`
- `_setPauseGuardian()`
- `_setMarketBorrowCaps()`
- `_setBorrowCapGuardian()`
- `_setProtocolPaused()`
- `_setVAIController()`
- `_setVAIMintRate()`
- `_setTreasuryData()`
- `_setBaseRate()`
- `_setFloatRate()`
- `_setReceiver()`
- `_grantXVS()`
- `_setVenusVAIVaultRate()`
- `_setVAIVaultInfo()`
- `_setVenusSpeed()`
- `_setComptrollerLens()`

Any compromise to the `admin` account may allow the hacker to take advantage of this authority and change important parameters of the contract and sabotage the whole system.

In the contract `Comptroller` the role `borrowCapGuardian` has authority over the functions shown in the diagram below.

Any compromise to the `borrowCapGuardian` account may allow the hacker to take advantage of this authority and set given borrow caps for given vToken markets.

![](https://accelerator-tasks-prod.s3.amazonaws.com/caf270c087f54708b85a518d2d309fbb/diagrams/centralization-Comptroller-borrowCapGuardian.svg)

In the contract `Comptroller` the role `pauseGuardian` has authority over the functions shown in the diagram below.

Any compromise to the `pauseGuardian` account may allow the hacker to take advantage of this authority and pause the protocol.

![](https://accelerator-tasks-prod.s3.amazonaws.com/caf270c087f54708b85a518d2d309fbb/diagrams/centralization-Comptroller-pauseGuardian.svg)

Centralization Risk in `Comptroller.sol`

A reentrancy attack can occur when the contract creates a function that makes an external call to another untrusted contract before resolving any effects. If the attacker can control the untrusted contract, they can make a recursive call back to the original function, repeating interactions that would have otherwise not run after the external call resolved the effects.

Check Effect Interaction Pattern Violated

When setting a new value of `treasuryPercentMantissa`, using the function `setTreasuryPercent()` from contract `Liquidator`; the potential new value is compared to `comptroller.liquidationIncentiveMantissa().sub(1e18)`. However, nothing in the `Comptroller` contract ensures that `liquidationIncentiveMantissa >= 1e18`.

`liquidationIncentiveMantissa` Should Be Greater Than 1e18

External calls are made inside a *for* loop. This might lead to a denial-of-service attack. If any of the calls revert, it will cause the entire loop to revert.

External Call Inside Loop

Addresses should be checked before assignment or external calls to make sure they are not zero addresses.

Missing Zero Address Validation

The return value of the `transfer()`/`transferFrom()` call is not checked.

Unchecked ERC-20 `transfer()`/`transferFrom()` Call

The below codes want to calculate the value of `actualAmount` which should be greater than `minReleaseAmount`. `_releaseAmount` is checked by `minReleaseAmount` but `xvsBalance` is not.
```solidity=1477
        uint256 actualAmount;
        uint256 deltaBlocks = sub_(getBlockNumber(), releaseStartBlock);
        // releaseAmount = venusVAIVaultRate * deltaBlocks
        uint256 _releaseAmount = mul_(venusVAIVaultRate, deltaBlocks);

        if (_releaseAmount < minReleaseAmount) {
            return;
        }

        if (xvsBalance >= _releaseAmount) {
            actualAmount = _releaseAmount;
        } else {
            actualAmount = xvsBalance;
        }
```
In the *else* block, `xvsBalance` should be checked to prevent having `actualAmount < minReleaseAmount`.

Logical issue of `actualAmount`

The comment :
```solidity=454
     * @param borrower The account which would repay the asset
```
is the same as the description of the `payer` parameter.

Wrong Comment

Each linked comment described a function and claim that it will revert on rejection. However, none of the related functions revert.

Related Functions Do Not Revert

The Natspec comments of the function `liquidateBorrowVerify()` does not include the description of the `uint seizeTokens` parameters.

Missing Parameter In Natspec Comments

Many functions have this comment:
```solidity
      * @return uint 0=success, otherwise a failure.
```
meaning that if something goes wrong the function will return a specific `uint` corresponding to the specific cause that made the function fail.
\
However, for `_setCloseFactor()` the only way it can fail is because of a **require** statement making the function revert and not returning any `uint`.

Unclear Meaning Of `failure` 

The Gas consumption of `memory` is not very significant as compared to the gas consumption of `storage`. In the linked pieces of code, `memory` could be used instead of `storage` since the object is never meant to be modified.

Inefficient `memory` & `storage` Management

The return value of an external call is not stored in a local or state variable.

Unused Return Value

The contract has an unlocked compiler version. An unlocked compiler version in the source code of the contract permits the user to compile it at or above a particular version. This, in turn, leads to differences in the generated bytecode between compilations due to different compiler versions. This can lead to ambiguity when debugging as compiler-specific bugs may occur in the codebase that would be hard to identify over a span of multiple compiler versions rather than a specific one.

Unlocked Compiler Version

The **require** can be used to check for conditions and throw an exception if the condition is not met. It is better to provide a string message containing details about the error that will be passed back to the caller.

Missing Error Messages

Boolean constants can be used directly and do not need to be compared to true or false.

Comparison to Boolean Constant

The linked state variables are never used in the codebase.

Unused State Variable

There should always be events emitted in the sensitive functions that are controlled by centralization roles.

Missing Emit Events

Many functions in the `Comptroller` contracts seem to be incomplete, therefore it is not possible to ensure the functional correctness of the system, including functions from other contracts which will rely on the future logic of those incomplete functions.

Functions Not Complete

In the contract `ComptrollerLens`:
- in the function `getVAIRepayRate()`:
```solidity=26                    
delta = sub_(1e18, oraclePrice, "VAI_REPAY_RATE_CALCULATION_FAILED");
```
can be replaced by 
```solidity                    
delta = 1e18 - oraclePrice;
```
because `1e18 >= oraclePrice`;
- in the function `getVAIRepayAmount()`:
```solidity=43
amount = mul_(rate, amount, "VAI_TOTAL_REPAY_AMOUNT_CALCULATION_FAILED");
amount = div_(amount, 1e18, "VAI_TOTAL_REPAY_AMOUNT_CALCULATION_FAILED");
```
can be replaced by:
```solidity
amount = rate * amount;
amount = amount / 1e18;
```
because `rate >= 1e18`.

Safe Arithmetic Function Unnecessary

In the contract `ComptrollerLens.sol`, the function `getVAIRepayRate()` uses an external function to get the `oraclePrice` value:
```solidity=19
          uint oraclePrice = Comptroller(comptroller)
              .oracle()
              .assetPrices(Comptroller(comptroller).vaiController().getVAIAddress());
```
unlike the other value obtain from an oracle in this contract, there is no check to ensure that `oraclePrice` is strictly greater than zero.

Is that intended?

Missing Check For `oraclePrice`

In the function `getVAIRepayRate()` if `oraclePrice == 1e18` then in the following piece of code:
```solidity=22
        if (1e18 >= oraclePrice) {
            uint delta;
            delta = sub_(1e18, oraclePrice, "VAI_REPAY_RATE_CALCULATION_FAILED");
            delta = mul_(delta, floatRateMantissa, "VAI_REPAY_RATE_CALCULATION_FAILED");
            delta = div_(delta, 1e18, "VAI_REPAY_RATE_CALCULATION_FAILED");
            rate = add_(rate, delta, "VAI_REPAY_RATE_CALCULATION_FAILED");
```
`delta` is always equal to zero and `rate` is not changed.

Unoptimized Check

The linked variables should be tight-packed.

Variable Tight-Packing

In the contract `Liquidator` the role `admin` has authority over the functions shown in the diagram below.

Any compromise to the `admin` account may allow the hacker to take advantage of this authority and sets a new percent of the seized amount that goes to the treasury.

![](https://accelerator-tasks-prod.s3.amazonaws.com/caf270c087f54708b85a518d2d309fbb/diagrams/centralization-Liquidator-admin.svg)

Centralization Risk in `Liquidator.sol`

In `requireNoError()`, the execution of function stop if `errCode == uint(0)`, however at the end of the function there is this **require** statement:
```solidity=186
    require(errCode == uint(0), string(fullMessage));
```
which can be evaluated only if `errCode != 0`.

Unnecesarry **require** Statement

The comment describing the `_liquidateVAI()` function should be 
```solidity
    /// @dev Transfers BEP20 tokens to self, then approves vai to take these tokens.
```

Inaccurate Comment

State variable from an abstract contract is shadowed.

Shadowing State Variable from Abstract Contracts

In `PriceOracle`, the function `assetPrices()` was not implemented within the scope of the contract for this audit.

Unimplemented Function

In the contract `VAIController` the role `admin` has authority over the functions shown in the diagram below.

Any compromise to the `admin` account may allow the hacker to take advantage of this authority and initialize the contract.

![](https://accelerator-tasks-prod.s3.amazonaws.com/caf270c087f54708b85a518d2d309fbb/diagrams/centralization-VAIController-admin.svg)

Centralization Risk in `VAIController.sol`

The error message should be `"mintVAIAmount cannot be zero"`.


Typo

The comment linked states that a `LiquidateBorrow` event is emitted however it is a `LiquidateVAI` event.

The Event Emitted Is `LiquidateVAI`

The linked statement does not have any use in the contract.

Useless Statement

Venus - Comptroller Audit

Comptroller.sol

PriceOracle.sol

VAIController.sol

ComptrollerLens.sol

ComptrollerStorage.sol

ComptrollerLensInterface.sol

VAIControllerInterface.sol

Liquidator.sol

VTokenInterfaces.sol

Incorrect Naming Convention Utilization

Public Function That Could Be Declared External

Lack Of Input Validation

Pull-Over-Push Pattern

Visibility Specifiers Missing

Redundant Variable Initialization

Function Optimization

Privileged Ownership

Unlocked Compiler Version Declaration

Venus XSV

XSV.sol

This report has been prepared for Venus Reward Token smart contracts, to discover issues and vulnerabilities in the source code of their Smart Contract as well as any contract dependencies that were not part of an officially recognized library. 

Venus Reward Token

Venus is a decentralized marketplace for lenders and borrowers with borderless stablecoins.

Venus

Mid Market Cap (25%-50% Amongst Projects)

Venus

Highlights & Alerts

This is just a placeholder

highlight placeholder lorem Ipsum...

highlight lorem

This is a alert demo

Alert demo

Code Audit History

36

0

19

5

12