Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

[VEN-1798]: Fix Certik 2023-08 findings for PSR/RiskFund/Shortfall contracts #287

Merged
merged 8 commits into from
Aug 16, 2023
Merged

[VEN-1798]: Fix Certik 2023-08 findings for PSR/RiskFund/Shortfall contracts #287

Show file tree
Hide file tree
Changes from 6 commits
Commits
Show all changes
8 commits
Select commit Hold shift + click to select a range
5ca628c
fix: [SSV-01] use _transferOutOrTrackDebt upon closing the auction
kkirka Aug 8, 2023
42a9f78
fix: [VPB-04] fix potential re-entrancy issues
kkirka Aug 8, 2023
9085787
refactor!: [RHR-01] make poolsAssetsReserves internal
kkirka Aug 9, 2023
5effe7b
docs: [VPB-05] fix typos and inconsistencies in docs
kkirka Aug 9, 2023
1dbf136
refactor: [SSV-04] simplify auction status checks
kkirka Aug 9, 2023
db0e468
fix: [RFR-01][RFR-03] unify the meaning of minAmountToConvert
kkirka Aug 10, 2023
e4820ff
fix: update method signature in access control check
kkirka Aug 14, 2023
6c559f1
fix: revert on approval failures
kkirka Aug 14, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
10 changes: 5 additions & 5 deletions contracts/RiskFund/ProtocolShareReserve.sol
View file
Open in desktop
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
Open in desktop
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
93 changes: 48 additions & 45 deletions contracts/RiskFund/RiskFund.sol
View file
Open in desktop
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 @@ -18,7 +19,7 @@ import { MaxLoopsLimitHelper } from "../MaxLoopsLimitHelper.sol";
import { ensureNonzeroAddress } from "../lib/validators.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.
Expand Down Expand Up @@ -154,7 +155,7 @@ contract RiskFund is AccessControlledV8, ExponentialNoError, ReserveHelpers, Max
uint256[] calldata amountsOutMin,
address[][] calldata paths,
uint256 deadline
) external override returns (uint256) {
) external override nonReentrant returns (uint256) {
_checkAccessAllowed("swapPoolsAssets(address[],uint256[],address[][])");
require(deadline >= block.timestamp, "Risk fund: deadline passed");
address poolRegistry_ = poolRegistry;
Expand All @@ -175,7 +176,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 +192,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 +234,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];
}
kkirka marked this conversation as resolved.
Show resolved Hide resolved

/**
Expand All @@ -255,48 +261,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];

ComptrollerViewInterface(comptroller).oracle().updatePrice(address(vToken));
uint256 balanceOfUnderlyingAsset = _poolsAssetsReserves[comptroller][underlyingAsset];

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).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;
}

return totalAmount;
Expand Down
25 changes: 12 additions & 13 deletions contracts/Shortfall/Shortfall.sol
View file
Open in desktop
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
39 changes: 20 additions & 19 deletions tests/hardhat/Fork/RiskFund.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -630,25 +630,26 @@ describe("Risk Fund: Tests", function () {
const riskFundUSDCBal = await USDC.balanceOf(riskFund.address);
expect(riskFundUSDCBal).equal(convertToUnit(9, 18));

const amount = await riskFund.callStatic.swapPoolsAssets(
[vUSDT.address, vUSDC.address, vUSDT2.address, vUSDC2.address, vUSDT3.address],
[
convertToUnit(10, 18),
convertToUnit(10, 18),
convertToUnit(10, 18),
convertToUnit(10, 18),
convertToUnit(10, 18),
],
[
[USDT.address, BUSD.address],
[USDC.address, BUSD.address],
[USDT.address, BUSD.address],
[USDC.address, BUSD.address],
[USDT.address, BUSD.address],
],
deadline,
);
expect(amount).equal("0");
await expect(
riskFund.swapPoolsAssets(
[vUSDT.address, vUSDC.address, vUSDT2.address, vUSDC2.address, vUSDT3.address],
[
convertToUnit(9, 18),
convertToUnit(9, 18),
convertToUnit(9, 18),
convertToUnit(9, 18),
convertToUnit(9, 18),
],
[
[USDT.address, BUSD.address],
[USDC.address, BUSD.address],
[USDT.address, BUSD.address],
[USDC.address, BUSD.address],
[USDT.address, BUSD.address],
],
deadline,
),
).to.be.revertedWith("RiskFund: minAmountToConvert violated");
});

it("Above min threshold amount", async function () {
Expand Down
Loading