Skip to content

Commit

Permalink
Merge pull request #287 from VenusProtocol/fix/funds-certik-2023-08
Browse files Browse the repository at this point in the history 
[VEN-1798]: Fix Certik 2023-08 findings for PSR/RiskFund/Shortfall contracts
  • Loading branch information
@kkirka
kkirka authored Aug 16, 2023
2 parents 8e2640e + 6c559f1 commit 381d991
Show file tree
Hide file tree
Showing 8 changed files with 223 additions and 91 deletions.
10 changes: 5 additions & 5 deletions contracts/RiskFund/ProtocolShareReserve.sol
View file
Original file line number Diff line number Diff line change
Expand Up @@ -73,27 +73,27 @@ contract ProtocolShareReserve is ExponentialNoError, ReserveHelpers, IProtocolSh
address comptroller,
address asset,
uint256 amount
) external returns (uint256) {
) external nonReentrant returns (uint256) {
ensureNonzeroAddress(asset);
require(amount <= poolsAssetsReserves[comptroller][asset], "ProtocolShareReserve: Insufficient pool balance");
require(amount <= _poolsAssetsReserves[comptroller][asset], "ProtocolShareReserve: Insufficient pool balance");

assetsReserves[asset] -= amount;
poolsAssetsReserves[comptroller][asset] -= amount;
_poolsAssetsReserves[comptroller][asset] -= amount;
uint256 protocolIncomeAmount = mul_(
Exp({ mantissa: amount }),
div_(Exp({ mantissa: PROTOCOL_SHARE_PERCENTAGE * EXP_SCALE }), BASE_UNIT)
).mantissa;

address riskFund_ = riskFund;

emit FundsReleased(comptroller, asset, amount);

IERC20Upgradeable(asset).safeTransfer(protocolIncome, protocolIncomeAmount);
IERC20Upgradeable(asset).safeTransfer(riskFund_, amount - protocolIncomeAmount);

// Update the pool asset's state in the risk fund for the above transfer.
IRiskFund(riskFund_).updateAssetsState(comptroller, asset);

emit FundsReleased(comptroller, asset, amount);

return amount;
}

Expand Down
8 changes: 4 additions & 4 deletions contracts/RiskFund/ReserveHelpers.sol
View file
Original file line number Diff line number Diff line change
Expand Up @@ -21,7 +21,7 @@ contract ReserveHelpers is Ownable2StepUpgradeable {

// Store the asset's reserve per pool in the ProtocolShareReserve.
// Comptroller(pool) -> Asset -> amount
mapping(address => mapping(address => uint256)) public poolsAssetsReserves;
mapping(address => mapping(address => uint256)) internal _poolsAssetsReserves;

// Address of pool registry contract
address public poolRegistry;
Expand Down Expand Up @@ -73,8 +73,8 @@ contract ReserveHelpers is Ownable2StepUpgradeable {
balanceDfference_ = balance_ - assetsReserves[_token];
}

IERC20Upgradeable(_token).safeTransfer(_to, balanceDfference_);
emit SweepToken(_token, _to, balanceDfference_);
IERC20Upgradeable(_token).safeTransfer(_to, balanceDfference_);
}

/**
Expand All @@ -87,7 +87,7 @@ contract ReserveHelpers is Ownable2StepUpgradeable {
function getPoolAssetReserve(address comptroller, address asset) external view returns (uint256) {
ensureNonzeroAddress(asset);
require(ComptrollerInterface(comptroller).isComptroller(), "ReserveHelpers: Comptroller address invalid");
return poolsAssetsReserves[comptroller][asset];
return _poolsAssetsReserves[comptroller][asset];
}

/**
Expand Down Expand Up @@ -115,7 +115,7 @@ contract ReserveHelpers is Ownable2StepUpgradeable {
balanceDifference = currentBalance - assetReserve;
}
assetsReserves[asset] += balanceDifference;
poolsAssetsReserves[comptroller][asset] += balanceDifference;
_poolsAssetsReserves[comptroller][asset] += balanceDifference;
emit AssetsReservesUpdated(comptroller, asset, balanceDifference);
}
}
Expand Down
98 changes: 52 additions & 46 deletions contracts/RiskFund/RiskFund.sol
View file
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,7 @@ pragma solidity 0.8.13;
import { IERC20Upgradeable } from "@openzeppelin/contracts-upgradeable/token/ERC20/IERC20Upgradeable.sol";
import { SafeERC20Upgradeable } from "@openzeppelin/contracts-upgradeable/token/ERC20/utils/SafeERC20Upgradeable.sol";
import { AccessControlledV8 } from "@venusprotocol/governance-contracts/contracts/Governance/AccessControlledV8.sol";
import { ResilientOracleInterface } from "@venusprotocol/oracle/contracts/interfaces/OracleInterface.sol";
import { ComptrollerInterface } from "../ComptrollerInterface.sol";
import { IRiskFund } from "./IRiskFund.sol";
import { ReserveHelpers } from "./ReserveHelpers.sol";
Expand All @@ -16,15 +17,18 @@ import { IPancakeswapV2Router } from "../IPancakeswapV2Router.sol";
import { IShortfall } from "../Shortfall/IShortfall.sol";
import { MaxLoopsLimitHelper } from "../MaxLoopsLimitHelper.sol";
import { ensureNonzeroAddress } from "../lib/validators.sol";
import { ApproveOrRevert } from "../lib/ApproveOrRevert.sol";

/**
* @title ReserveHelpers
* @title RiskFund
* @author Venus
* @notice Contract with basic features to track/hold different assets for different Comptrollers.
* @dev This contract does not support BNB.
*/
contract RiskFund is AccessControlledV8, ExponentialNoError, ReserveHelpers, MaxLoopsLimitHelper, IRiskFund {
using SafeERC20Upgradeable for IERC20Upgradeable;
using ApproveOrRevert for IERC20Upgradeable;

address public convertibleBaseAsset;
address public shortfall;
address public pancakeSwapRouter;
Expand Down Expand Up @@ -154,8 +158,8 @@ contract RiskFund is AccessControlledV8, ExponentialNoError, ReserveHelpers, Max
uint256[] calldata amountsOutMin,
address[][] calldata paths,
uint256 deadline
) external override returns (uint256) {
_checkAccessAllowed("swapPoolsAssets(address[],uint256[],address[][])");
) external override nonReentrant returns (uint256) {
_checkAccessAllowed("swapPoolsAssets(address[],uint256[],address[][],uint256)");
require(deadline >= block.timestamp, "Risk fund: deadline passed");
address poolRegistry_ = poolRegistry;
ensureNonzeroAddress(poolRegistry_);
Expand All @@ -175,7 +179,7 @@ contract RiskFund is AccessControlledV8, ExponentialNoError, ReserveHelpers, Max
require(Comptroller(comptroller).isMarketListed(VToken(markets[i])), "market is not listed");

uint256 swappedTokens = _swapAsset(VToken(markets[i]), comptroller, amountsOutMin[i], paths[i]);
poolsAssetsReserves[comptroller][convertibleBaseAsset] += swappedTokens;
_poolsAssetsReserves[comptroller][convertibleBaseAsset] += swappedTokens;
assetsReserves[convertibleBaseAsset] += swappedTokens;
totalAmount = totalAmount + swappedTokens;
}
Expand All @@ -191,24 +195,29 @@ contract RiskFund is AccessControlledV8, ExponentialNoError, ReserveHelpers, Max
* @param amount Amount to be transferred to auction contract.
* @return Number reserved tokens.
*/
function transferReserveForAuction(address comptroller, uint256 amount) external override returns (uint256) {
function transferReserveForAuction(address comptroller, uint256 amount)
external
override
nonReentrant
returns (uint256)
{
address shortfall_ = shortfall;
require(msg.sender == shortfall_, "Risk fund: Only callable by Shortfall contract");
require(
amount <= poolsAssetsReserves[comptroller][convertibleBaseAsset],
amount <= _poolsAssetsReserves[comptroller][convertibleBaseAsset],
"Risk Fund: Insufficient pool reserve."
);
unchecked {
poolsAssetsReserves[comptroller][convertibleBaseAsset] =
poolsAssetsReserves[comptroller][convertibleBaseAsset] -
_poolsAssetsReserves[comptroller][convertibleBaseAsset] =
_poolsAssetsReserves[comptroller][convertibleBaseAsset] -
amount;
}
unchecked {
assetsReserves[convertibleBaseAsset] = assetsReserves[convertibleBaseAsset] - amount;
}
IERC20Upgradeable(convertibleBaseAsset).safeTransfer(shortfall_, amount);

emit TransferredReserveForAuction(comptroller, amount);
IERC20Upgradeable(convertibleBaseAsset).safeTransfer(shortfall_, amount);

return amount;
}
Expand All @@ -228,7 +237,7 @@ contract RiskFund is AccessControlledV8, ExponentialNoError, ReserveHelpers, Max
*/
function getPoolsBaseAssetReserves(address comptroller) external view returns (uint256) {
require(ComptrollerInterface(comptroller).isComptroller(), "Risk Fund: Comptroller address invalid");
return poolsAssetsReserves[comptroller][convertibleBaseAsset];
return _poolsAssetsReserves[comptroller][convertibleBaseAsset];
}

/**
Expand All @@ -255,48 +264,45 @@ contract RiskFund is AccessControlledV8, ExponentialNoError, ReserveHelpers, Max
address[] calldata path
) internal returns (uint256) {
require(amountOutMin != 0, "RiskFund: amountOutMin must be greater than 0 to swap vToken");
require(amountOutMin >= minAmountToConvert, "RiskFund: amountOutMin should be greater than minAmountToConvert");
uint256 totalAmount;

address underlyingAsset = vToken.underlying();
address convertibleBaseAsset_ = convertibleBaseAsset;
uint256 balanceOfUnderlyingAsset = poolsAssetsReserves[comptroller][underlyingAsset];
uint256 balanceOfUnderlyingAsset = _poolsAssetsReserves[comptroller][underlyingAsset];

ComptrollerViewInterface(comptroller).oracle().updatePrice(address(vToken));

uint256 underlyingAssetPrice = ComptrollerViewInterface(comptroller).oracle().getUnderlyingPrice(
address(vToken)
);
if (balanceOfUnderlyingAsset == 0) {
return 0;
}

if (balanceOfUnderlyingAsset > 0) {
Exp memory oraclePrice = Exp({ mantissa: underlyingAssetPrice });
uint256 amountInUsd = mul_ScalarTruncate(oraclePrice, balanceOfUnderlyingAsset);

if (amountInUsd >= minAmountToConvert) {
assetsReserves[underlyingAsset] -= balanceOfUnderlyingAsset;
poolsAssetsReserves[comptroller][underlyingAsset] -= balanceOfUnderlyingAsset;

if (underlyingAsset != convertibleBaseAsset_) {
require(path[0] == underlyingAsset, "RiskFund: swap path must start with the underlying asset");
require(
path[path.length - 1] == convertibleBaseAsset_,
"RiskFund: finally path must be convertible base asset"
);
address pancakeSwapRouter_ = pancakeSwapRouter;
IERC20Upgradeable(underlyingAsset).approve(pancakeSwapRouter_, 0);
IERC20Upgradeable(underlyingAsset).approve(pancakeSwapRouter_, balanceOfUnderlyingAsset);
uint256[] memory amounts = IPancakeswapV2Router(pancakeSwapRouter_).swapExactTokensForTokens(
balanceOfUnderlyingAsset,
amountOutMin,
path,
address(this),
block.timestamp
);
totalAmount = amounts[path.length - 1];
} else {
totalAmount = balanceOfUnderlyingAsset;
}
}
ResilientOracleInterface oracle = ComptrollerViewInterface(comptroller).oracle();
oracle.updateAssetPrice(convertibleBaseAsset_);
Exp memory baseAssetPrice = Exp({ mantissa: oracle.getPrice(convertibleBaseAsset_) });
uint256 amountOutMinInUsd = mul_ScalarTruncate(baseAssetPrice, amountOutMin);

require(amountOutMinInUsd >= minAmountToConvert, "RiskFund: minAmountToConvert violated");

assetsReserves[underlyingAsset] -= balanceOfUnderlyingAsset;
_poolsAssetsReserves[comptroller][underlyingAsset] -= balanceOfUnderlyingAsset;

if (underlyingAsset != convertibleBaseAsset_) {
require(path[0] == underlyingAsset, "RiskFund: swap path must start with the underlying asset");
require(
path[path.length - 1] == convertibleBaseAsset_,
"RiskFund: finally path must be convertible base asset"
);
address pancakeSwapRouter_ = pancakeSwapRouter;
IERC20Upgradeable(underlyingAsset).approveOrRevert(pancakeSwapRouter_, 0);
IERC20Upgradeable(underlyingAsset).approveOrRevert(pancakeSwapRouter_, balanceOfUnderlyingAsset);
uint256[] memory amounts = IPancakeswapV2Router(pancakeSwapRouter_).swapExactTokensForTokens(
balanceOfUnderlyingAsset,
amountOutMin,
path,
address(this),
block.timestamp
);
totalAmount = amounts[path.length - 1];
} else {
totalAmount = balanceOfUnderlyingAsset;
}

return totalAmount;
Expand Down
25 changes: 12 additions & 13 deletions contracts/Shortfall/Shortfall.sol
View file
Original file line number Diff line number Diff line change
Expand Up @@ -86,13 +86,13 @@ contract Shortfall is
/// @notice Incentive to auction participants, initial value set to 1000 or 10%
uint256 public incentiveBps;

/// @notice Time to wait for next bidder. initially waits for 10 blocks
/// @notice Time to wait for next bidder. Initially waits for 100 blocks
uint256 public nextBidderBlockLimit;

/// @notice Boolean of if auctions are paused
bool public auctionsPaused;

/// @notice Time to wait for first bidder. initially waits for 100 blocks
/// @notice Time to wait for first bidder. Initially waits for 100 blocks
uint256 public waitForFirstBidder;

/// @notice base asset contract address
Expand Down Expand Up @@ -286,7 +286,7 @@ contract Shortfall is
}

uint256 transferredAmount = riskFund.transferReserveForAuction(comptroller, riskFundBidAmount);
IERC20Upgradeable(convertibleBaseAsset).safeTransfer(auction.highestBidder, riskFundBidAmount);
_transferOutOrTrackDebt(IERC20Upgradeable(convertibleBaseAsset), auction.highestBidder, riskFundBidAmount);

emit AuctionClosed(
comptroller,
Expand All @@ -305,7 +305,7 @@ contract Shortfall is
* @custom:event Emits AuctionStarted event on success
* @custom:event Errors if auctions are paused
*/
function startAuction(address comptroller) external {
function startAuction(address comptroller) external nonReentrant {
require(!auctionsPaused, "Auctions are paused");
_startAuction(comptroller);
}
Expand All @@ -315,7 +315,7 @@ contract Shortfall is
* @param comptroller Address of the pool
* @custom:event Emits AuctionRestarted event on successful restart
*/
function restartAuction(address comptroller) external {
function restartAuction(address comptroller) external nonReentrant {
Auction storage auction = auctions[comptroller];

require(!auctionsPaused, "auctions are paused");
Expand All @@ -332,7 +332,7 @@ contract Shortfall is
* @notice Update next bidder block limit which is used determine when an auction can be closed
* @param _nextBidderBlockLimit New next bidder block limit
* @custom:event Emits NextBidderBlockLimitUpdated on success
* @custom:access Restricted to owner
* @custom:access Restricted by ACM
*/
function updateNextBidderBlockLimit(uint256 _nextBidderBlockLimit) external {
_checkAccessAllowed("updateNextBidderBlockLimit(uint256)");
Expand All @@ -343,10 +343,10 @@ contract Shortfall is
}

/**
* @notice Updates the inventive BPS
* @notice Updates the incentive BPS
* @param _incentiveBps New incentive BPS
* @custom:event Emits IncentiveBpsUpdated on success
* @custom:access Restricted to owner
* @custom:access Restricted by ACM
*/
function updateIncentiveBps(uint256 _incentiveBps) external {
_checkAccessAllowed("updateIncentiveBps(uint256)");
Expand All @@ -360,7 +360,7 @@ contract Shortfall is
* @notice Update minimum pool bad debt to start auction
* @param _minimumPoolBadDebt Minimum bad debt in BUSD for a pool to start auction
* @custom:event Emits MinimumPoolBadDebtUpdated on success
* @custom:access Restricted to owner
* @custom:access Restricted by ACM
*/
function updateMinimumPoolBadDebt(uint256 _minimumPoolBadDebt) external {
_checkAccessAllowed("updateMinimumPoolBadDebt(uint256)");
Expand All @@ -373,7 +373,7 @@ contract Shortfall is
* @notice Update wait for first bidder block count. If the first bid is not made within this limit, the auction is closed and needs to be restarted
* @param _waitForFirstBidder New wait for first bidder block count
* @custom:event Emits WaitForFirstBidderUpdated on success
* @custom:access Restricted to owner
* @custom:access Restricted by ACM
*/
function updateWaitForFirstBidder(uint256 _waitForFirstBidder) external {
_checkAccessAllowed("updateWaitForFirstBidder(uint256)");
Expand Down Expand Up @@ -433,8 +433,7 @@ contract Shortfall is

Auction storage auction = auctions[comptroller];
require(
(auction.startBlock == 0 && auction.status == AuctionStatus.NOT_STARTED) ||
auction.status == AuctionStatus.ENDED,
auction.status == AuctionStatus.NOT_STARTED || auction.status == AuctionStatus.ENDED,
"auction is on-going"
);

Expand Down Expand Up @@ -530,7 +529,7 @@ contract Shortfall is
* @return True if the auction has started
*/
function _isStarted(Auction storage auction) internal view returns (bool) {
return auction.startBlock != 0 && auction.status == AuctionStatus.STARTED;
return auction.status == AuctionStatus.STARTED;
}

/**
Expand Down
34 changes: 34 additions & 0 deletions contracts/lib/ApproveOrRevert.sol
View file
Original file line number Diff line number Diff line change
@@ -0,0 +1,34 @@
// SPDX-License-Identifier: MIT

pragma solidity 0.8.13;

import { IERC20Upgradeable } from "@openzeppelin/contracts-upgradeable/token/ERC20/IERC20Upgradeable.sol";

library ApproveOrRevert {
/// @notice Thrown if a contract is unable to approve a transfer
error ApproveFailed();

/// @notice Approves a transfer, ensuring that it is successful. This function supports non-compliant
/// tokens like the ones that don't return a boolean value on success. Thus, such approve call supports
/// three different kinds of tokens:
/// * Compliant tokens that revert on failure
/// * Compliant tokens that return false on failure
/// * Non-compliant tokens that don't return a value
/// @param token The contract address of the token which will be transferred
/// @param spender The spender contract address
/// @param amount The value of the transfer
function approveOrRevert(
IERC20Upgradeable token,
address spender,
uint256 amount
) internal {
bytes memory callData = abi.encodeCall(token.approve, (spender, amount));

// solhint-disable-next-line avoid-low-level-calls
(bool success, bytes memory result) = address(token).call(callData);

if (!success || (result.length != 0 && !abi.decode(result, (bool)))) {
revert ApproveFailed();
}
}
}
18 changes: 18 additions & 0 deletions contracts/test/lib/ApproveOrRevertHarness.sol
View file
Original file line number Diff line number Diff line change
@@ -0,0 +1,18 @@
// SPDX-License-Identifier: MIT

pragma solidity 0.8.13;

import { IERC20Upgradeable } from "@openzeppelin/contracts-upgradeable/token/ERC20/IERC20Upgradeable.sol";
import { ApproveOrRevert } from "../../lib/ApproveOrRevert.sol";

contract ApproveOrRevertHarness {
using ApproveOrRevert for IERC20Upgradeable;

function approve(
IERC20Upgradeable token,
address spender,
uint256 amount
) external {
token.approveOrRevert(spender, amount);
}
}
Loading

0 comments on commit 381d991

Please sign in to comment.