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 3 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
25 changes: 15 additions & 10 deletions contracts/RiskFund/RiskFund.sol
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -154,7 +154,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 +175,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 +191,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 +233,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 Down Expand Up @@ -260,7 +265,7 @@ contract RiskFund is AccessControlledV8, ExponentialNoError, ReserveHelpers, Max

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

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

Expand All @@ -274,7 +279,7 @@ contract RiskFund is AccessControlledV8, ExponentialNoError, ReserveHelpers, Max

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

if (underlyingAsset != convertibleBaseAsset_) {
require(path[0] == underlyingAsset, "RiskFund: swap path must start with the underlying asset");
Expand Down
6 changes: 3 additions & 3 deletions contracts/Shortfall/Shortfall.sol
View file
Open in desktop
Original file line number Diff line number Diff line change
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 Down