Venus Protocol Isolated Pools - lfzkoala's results

Issue Name: Ambiguous Error (Minor) In the enterMarkets function, we assign NO_ERROR = 0 to the results[i] for those vToken added, but the default value of an array item in solidity is also 0 (according to this link https://solidity-kr.readthedocs.io/ko/latest/control-structures.html#:~:text=The%20default%20value%20for%20the,an%20empty%20array%20or%20string.). So it seems there is not difference of the the results[i] value when vToken is successfully added to markets or not. To resolve it, choose another value assigned to NO_ERROR. It’s hard to evaluate the impact now because it can be seen only when the enterMarkets function is used/deployed in practice.

Issue Name: Missing Input Validation (minor) Description of the Security Issue: The function _distributeSupplierRewardToken does not validate its inputs vToken and supplier. This could potentially lead to unexpected behavior if incorrect parameters are passed in. This issue also works for _updateRewardTokenSupplyIndex function. Proof of Concept of Attack: If a malicious actor had control of the contract calling this function, they could potentially pass in a malicious vToken contract that has a balanceOf function with unexpected side effects. Location of the Security Issue:

function _distributeSupplierRewardToken(address vToken, address supplier) internal {
    //...
    uint256 supplierTokens = VToken(vToken).balanceOf(supplier);
    //...
}

How to Resolve the Security Issue: Validate that vToken and supplier are non-zero addresses and that vToken is a trusted contract address before proceeding with the function. Additionally, it's recommended to use a registry or whitelist of trusted token contracts.

require(vToken != address(0), "Invalid vToken address");
require(supplier != address(0), "Invalid supplier address");
require(isTrustedToken(vToken), "vToken is not trusted");

Issue Name: Potential Redundant Code In the addRewardsDistributor function, the code assigns a new variable rewardsDistributorsLen = rewardsDistributors.length and check _ensureMaxLoops(rewardsDistributorsLen+1), but in the following code, the variable rewardsDistributorsLen is never used, so it doesn’t make sense to check the max loops for rewardsDistributorsLen+1 because the original goal to ensure max loops is to prevent DOS for loop iteration.

Issue Name - Lack Input Validation on owner (Minor) In the balanceOfUnderlying function, the function doesn’t validate the address of the owner. This can be fixed by checking on address(owner) != address(0)

Issue Name: Inadequate Check for Zero Address in _swapAsset Issue risk level: Low

Description of the security issue: In the _swapAsset function, it is assumed that the underlyingAsset of a VToken is not a zero address. There is no explicit check for this.

Proof of Concept of Attack with code example: If an attacker creates a malicious VToken contract with the underlying() function returning a zero address, and this malicious contract somehow gets into the markets array passed to swapPoolsAssets, it could cause unexpected behavior in the _swapAsset function.

Location of the security issue with code snippet:

address underlyingAsset = VTokenInterface(address(vToken)).underlying();

How to resolve the security issue with code example: Check that the underlyingAsset is not a zero address.

require(underlyingAsset != address(0), "RiskFund: underlying asset is a zero address");

Issue Name: Inadequate Checks on Array Sizes in swapPoolsAssets

Description of the security issue: In the swapPoolsAssets function, the input arguments include three arrays: markets, amountsOutMin, and paths. The function only checks whether the lengths of markets and amountsOutMin, and markets and paths are equal, but does not check whether the lengths of amountsOutMin and paths are equal.

Proof of Concept of Attack with code example: If an attacker passes in arrays such that markets.length equals amountsOutMin.length, and markets.length equals paths.length, but amountsOutMin.length does not equal paths.length, the function will not throw an error, and unexpected behavior may occur.

Location of the security issue with code snippet:

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");

How to resolve the security issue with code example: Add a require statement to ensure that the lengths of amountsOutMin and paths are also equal.

require(amountsOutMin.length == paths.length, "Risk fund: amountsOutMin and paths are unequal lengths");

Issue Name: Potential Re-entrancy Vulnerability

Description of the security issue: In the updateAssetsState function, IERC20Upgradeable(asset).balanceOf(address(this)) can potentially cause a re-entrancy attack if the asset is a malicious contract.

Proof of Concept of Attack with code example: The malicious contract can call back into ReserveHelpers during the execution of the balanceOf function.

contract MaliciousToken {
    ReserveHelpers reserveHelpers;

    constructor(ReserveHelpers _reserveHelpers) {
        reserveHelpers = _reserveHelpers;
    }

    function balanceOf(address account) public returns (uint256) {
        reserveHelpers.updateAssetsState(address(1), address(this));
        return 1000000;
    }
}

Location of the security issue with code snippet:

uint256 currentBalance = IERC20Upgradeable(asset).balanceOf(address(this));

How to resolve the security issue with code example: Use the Checks-Effects-Interactions pattern to prevent re-entrancy. Also, consider using OpenZeppelin's ReentrancyGuard contract for additional protection.

uint256 currentBalance;
unchecked {
    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);

Issue Name - Error not used In the ErrorReporter.sol file, there are multiple errors that are not used yet. For example, TransferComptrollerRejection, TransferNotEnough, TransferTooMuch.etc. Double check whether you still need these defined errors and then clean this file.

Issue Name: Division Before Multiplication in getBorrowRate and getSupplyRate methods

Description of the security issue: Division before multiplication can lead to an underflow which could be exploited to get incorrect interest rates.

Proof of Concept of Attack with code example: There's no direct exploit available for this issue. However, it can lead to mathematical inaccuracies.

Location of the security issue with code snippet: In the getBorrowRate and getSupplyRate functions:

return ((ur * multiplierPerBlock) / BASE) + baseRatePerBlock;

uint256 rateToPool = (borrowRate * oneMinusReserveFactor) / BASE;

How to resolve the security issue with code example: Multiply before dividing to ensure precision and prevent underflow:

return ((ur * multiplierPerBlock + BASE - 1) / BASE) + baseRatePerBlock;

uint256 rateToPool = (borrowRate * oneMinusReserveFactor + BASE - 1) / BASE;

**Issue 1: Unrestricted Proxy Deployment**

Issue risk level: Medium

Description of the security issue: The `deployVTokenProxy` function is not protected by any access control mechanism. This means that anyone can call the function and potentially deploy proxies with arbitrary parameters.

Proof of Concept of Attack with code example: An attacker could call `deployVTokenProxy` with malicious parameters to deploy proxy contracts that could be manipulated or abused.

Location of the security issue with code snippet: In the `deployVTokenProxy` function:
```solidity
function deployVTokenProxy(VTokenArgs memory input) external returns (VToken) {
    // ... implementation ...
}

How to resolve the security issue with code example: Implement access control, such as the 'onlyOwner' modifier from the OpenZeppelin library:

function deployVTokenProxy(VTokenArgs memory input) external onlyOwner returns (VToken) {
    // ... implementation ...
}

Issue Name: No Validation of Arguments in VTokenArgs**

Description of the security issue: There is no validation on the arguments passed to the deployVTokenProxy function. Providing invalid arguments could lead to proxies being deployed with incorrect configurations.

Proof of Concept of Attack with code example: An attacker could pass in an incorrect or malicious beaconAddress or underlying_ address when calling deployVTokenProxy, leading to proxy contracts that are incorrectly configured or potentially malicious.

Location of the security issue with code snippet: In the deployVTokenProxy function:

function deployVTokenProxy(VTokenArgs memory input) external returns (VToken) {
    // ... implementation ...
}

How to resolve the security issue with code example: Add checks to validate the arguments:

function deployVTokenProxy(VTokenArgs memory input) external onlyOwner returns (VToken) {
    require(input.underlying_ != address(0), "Invalid underlying asset address");
    require(input.beaconAddress != address(0), "Invalid beacon address");
    // ... additional checks and rest of the implementation ...
}