Skip to content

Commit

Permalink
Merge pull request #11 from Gearbox-protocol/poolQuotaKeeper-improvem…
Browse files Browse the repository at this point in the history 
…ents

fix: pqk improvements + tests
  • Loading branch information
@0xmikko
0xmikko authored Mar 29, 2023
2 parents 6ced564 + ed89055 commit 5dd3b9f
Show file tree
Hide file tree
Showing 30 changed files with 951 additions and 494 deletions.
7 changes: 2 additions & 5 deletions contracts/adapters/AbstractAdapter.sol
View file
Original file line number Diff line number Diff line change
Expand Up @@ -26,12 +26,9 @@ abstract contract AbstractAdapter is IAdapter, ACLNonReentrantTrait {
/// @param _creditManager Credit Manager to connect this adapter to
/// @param _targetContract Address of the contract this adapter should interact with
constructor(address _creditManager, address _targetContract)
ACLNonReentrantTrait(IPool4626(ICreditManagerV2(_creditManager).pool()).addressProvider())
ACLNonReentrantTrait(address(IPool4626(ICreditManagerV2(_creditManager).pool()).addressProvider()))
nonZeroAddress(_targetContract) // F: [AA-2]
{
if (_targetContract == address(0)) {
revert ZeroAddressException(); // F: [AA-2]
}

creditManager = ICreditManagerV2(_creditManager); // F: [AA-1]
addressProvider = IAddressProvider(IPool4626(creditManager.pool()).addressProvider()); // F: [AA-1]
targetContract = _targetContract; // F: [AA-1]
Expand Down
15 changes: 12 additions & 3 deletions contracts/core/ACLNonReentrantTrait.sol
View file
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,7 @@ pragma solidity ^0.8.10;

import {Pausable} from "@openzeppelin/contracts/security/Pausable.sol";
import {AddressProvider} from "@gearbox-protocol/core-v2/contracts/core/AddressProvider.sol";
import {ContractsRegister} from "@gearbox-protocol/core-v2/contracts/core/ContractsRegister.sol";
import {IACL} from "@gearbox-protocol/core-v2/contracts/interfaces/IACL.sol";
import {
ZeroAddressException,
Expand Down Expand Up @@ -63,11 +64,14 @@ abstract contract ACLNonReentrantTrait is Pausable {

event NewController(address indexed newController);

modifier nonZeroAddress(address addr) {
if (addr == address(0)) revert ZeroAddressException(); // F:[P4-2]
_;
}

/// @dev constructor
/// @param addressProvider Address of address repository
constructor(address addressProvider) {
if (addressProvider == address(0)) revert ZeroAddressException(); // F:[AA-2]

constructor(address addressProvider) nonZeroAddress(addressProvider) {
_acl = IACL(AddressProvider(addressProvider).getACL());
controller = IACL(AddressProvider(addressProvider).getACL()).owner();
}
Expand All @@ -94,6 +98,11 @@ abstract contract ACLNonReentrantTrait is Pausable {
_;
}

// modifier registeredCreditManagerOnly(address creditManager) {
// if (!_cr.isCreditManager(creditManager)) revert();
// _;
// }

///@dev Pause contract
function pause() external {
if (!_acl.isPausableAdmin(msg.sender)) {
Expand Down
41 changes: 18 additions & 23 deletions contracts/credit/CreditConfigurator.sol
View file
Original file line number Diff line number Diff line change
Expand Up @@ -35,11 +35,11 @@ import {IPoolQuotaKeeper} from "../interfaces/IPoolQuotaKeeper.sol";

// EXCEPTIONS
import {
ZeroAddressException,
AddressIsNotContractException,
IncorrectPriceFeedException,
IncorrectTokenContractException,
CallerNotPausableAdminException
CallerNotPausableAdminException,
TokenNotAllowedException
} from "../interfaces/IErrors.sol";
import {ICreditManagerV2, ICreditManagerV2Exceptions} from "../interfaces/ICreditManagerV2.sol";

Expand Down Expand Up @@ -161,9 +161,8 @@ contract CreditConfigurator is ICreditConfigurator, ACLNonReentrantTrait {

/// @dev Makes all sanity checks and adds the token to the collateral token list
/// @param token Address of token to be added
function addCollateralToken(address token) internal {
function addCollateralToken(address token) internal nonZeroAddress(token) {
// Checks that token != address(0)
if (token == address(0)) revert ZeroAddressException(); // F:[CC-3]

if (!token.isContract()) revert AddressIsNotContractException(token); // F:[CC-3]

Expand Down Expand Up @@ -328,7 +327,7 @@ contract CreditConfigurator is ICreditConfigurator, ACLNonReentrantTrait {
// tokenMask can't be 1, since this mask is reserved for underlying

if (tokenMask == 0 || tokenMask == 1) {
revert ICreditManagerV2Exceptions.TokenNotAllowedException();
revert TokenNotAllowedException();
} // F:[CC-7]
}

Expand All @@ -349,9 +348,8 @@ contract CreditConfigurator is ICreditConfigurator, ACLNonReentrantTrait {
}

/// @dev IMPLEMENTATION: allowContract
function _allowContract(address targetContract, address adapter) internal {
function _allowContract(address targetContract, address adapter) internal nonZeroAddress(targetContract) {
// Checks that targetContract or adapter != address(0)
if (targetContract == address(0)) revert ZeroAddressException(); // F:[CC-12]

if (!targetContract.isContract() && (targetContract != UNIVERSAL_CONTRACT)) {
revert AddressIsNotContractException(targetContract);
Expand Down Expand Up @@ -397,10 +395,8 @@ contract CreditConfigurator is ICreditConfigurator, ACLNonReentrantTrait {
external
override
controllerOnly // F:[CC-2B]
nonZeroAddress(targetContract) // F:[CC-12]
{
// Checks that targetContract is not address(0)
if (targetContract == address(0)) revert ZeroAddressException(); // F:[CC-12]

// Checks that targetContract has a connected adapter
address adapter = creditManager.contractToAdapter(targetContract);
if (adapter == address(0)) {
Expand All @@ -423,10 +419,12 @@ contract CreditConfigurator is ICreditConfigurator, ACLNonReentrantTrait {
/// Useful to remove "orphaned" adapters, i.e. adapters that were replaced but still point
/// to the contract for some reason. This allows users to still execute actions through the old adapter,
/// even though that is not intended.
function forbidAdapter(address adapter) external override configuratorOnly {
/// Sanity check that zero address was not passed
if (adapter == address(0)) revert ZeroAddressException(); // F: [CC-40]

function forbidAdapter(address adapter)
external
override
configuratorOnly
nonZeroAddress(adapter) // F: [CC-40]
{
/// If the adapter already has no linked target contract, then there is nothing to change
address targetContract = creditManager.adapterToContract(adapter);
if (targetContract == address(0)) {
Expand Down Expand Up @@ -652,10 +650,11 @@ contract CreditConfigurator is ICreditConfigurator, ACLNonReentrantTrait {

/// @dev Performs sanity checks that the address is a contract compatible
/// with the current Credit Manager
function _revertIfContractIncompatible(address _contract) internal view {
// Checks that the address is not zero address
if (_contract == address(0)) revert ZeroAddressException(); // F:[CC-12,29]

function _revertIfContractIncompatible(address _contract)
internal
view
nonZeroAddress(_contract) // F:[CC-12,29]
{
// Checks that the address is a contract
if (!_contract.isContract()) {
revert AddressIsNotContractException(_contract);
Expand Down Expand Up @@ -768,13 +767,9 @@ contract CreditConfigurator is ICreditConfigurator, ACLNonReentrantTrait {
_setBotList(botList);
}

function _setBotList(address botList) internal {
function _setBotList(address botList) internal nonZeroAddress(botList) {
address currentBotList = creditFacade().botList();

if (botList == address(0)) {
revert ZeroAddressException();
}

if (botList != currentBotList) {
creditFacade().setBotList(botList);
emit BotListUpdated(botList);
Expand Down
32 changes: 12 additions & 20 deletions contracts/credit/CreditFacade.sol
View file
Original file line number Diff line number Diff line change
Expand Up @@ -6,7 +6,7 @@ pragma solidity ^0.8.10;
import {IERC20} from "@openzeppelin/contracts/token/ERC20/IERC20.sol";
import {Address} from "@openzeppelin/contracts/utils/Address.sol";
import {EnumerableSet} from "@openzeppelin/contracts/utils/structs/EnumerableSet.sol";
import {ReentrancyGuard} from "@openzeppelin/contracts/security/ReentrancyGuard.sol";
import {ACLNonReentrantTrait} from "../core/ACLNonReentrantTrait.sol";

// DATA
import {MultiCall} from "@gearbox-protocol/core-v2/contracts/libraries/MultiCall.sol";
Expand All @@ -17,6 +17,7 @@ import {QuotaUpdate} from "../interfaces/IPoolQuotaKeeper.sol";
import {ICreditFacade, ICreditFacadeExtended, FullCheckParams} from "../interfaces/ICreditFacade.sol";
import {ICreditManagerV2, ClosureAction} from "../interfaces/ICreditManagerV2.sol";
import {IPriceOracleV2} from "@gearbox-protocol/core-v2/contracts/interfaces/IPriceOracle.sol";
import {IPool4626} from "../interfaces/IPool4626.sol";
import {IDegenNFT} from "@gearbox-protocol/core-v2/contracts/interfaces/IDegenNFT.sol";
import {IWETH} from "@gearbox-protocol/core-v2/contracts/interfaces/external/IWETH.sol";
import {IWETHGateway} from "../interfaces/IWETHGateway.sol";
Expand All @@ -28,9 +29,6 @@ import {IBotList} from "../interfaces/IBotList.sol";
import {LEVERAGE_DECIMALS} from "@gearbox-protocol/core-v2/contracts/libraries/Constants.sol";
import {PERCENTAGE_FACTOR} from "@gearbox-protocol/core-v2/contracts/libraries/PercentageMath.sol";

// EXCEPTIONS
import {ZeroAddressException} from "../interfaces/IErrors.sol";

struct Params {
/// @dev Maximal amount of new debt that can be taken per block
uint128 maxBorrowedAmountPerBlock;
Expand All @@ -54,7 +52,7 @@ struct Limits {
/// - Through CreditFacade, which provides all the required account management function: open / close / liquidate / manageDebt,
/// as well as Multicalls that allow to perform multiple actions within a single transaction, with a single health check
/// - Through adapters, which call the Credit Manager directly, but only allow interactions with specific target contracts
contract CreditFacade is ICreditFacade, ReentrancyGuard {
contract CreditFacade is ICreditFacade, ACLNonReentrantTrait {
using EnumerableSet for EnumerableSet.AddressSet;
using Address for address;

Expand All @@ -79,12 +77,12 @@ contract CreditFacade is ICreditFacade, ReentrancyGuard {
/// @dev Address of the underlying token
address public immutable underlying;

/// @dev A map that stores whether a user allows a transfer of an account from another user to themselves
mapping(address => mapping(address => bool)) public override transfersAllowed;

/// @dev Contract containing the list of approval statuses for borrowers / bots
address public botList;

/// @dev A map that stores whether a user allows a transfer of an account from another user to themselves
mapping(address => mapping(address => bool)) public override transfersAllowed;

/// @dev Address of WETH
address public immutable wethAddress;

Expand Down Expand Up @@ -118,10 +116,10 @@ contract CreditFacade is ICreditFacade, ReentrancyGuard {
/// @param _blacklistHelper address of the funds recovery contract for blacklistable underlyings.
/// Must be address(0) is the underlying is not blacklistable
/// @param _expirable Whether the CreditFacade can expire and implements expiration-related logic
constructor(address _creditManager, address _degenNFT, address _blacklistHelper, bool _expirable) {
// Additional check that _creditManager is not address(0)
if (_creditManager == address(0)) revert ZeroAddressException(); // F:[FA-1]

constructor(address _creditManager, address _degenNFT, address _blacklistHelper, bool _expirable)
ACLNonReentrantTrait(address(IPool4626(ICreditManagerV2(_creditManager).pool()).addressProvider()))
nonZeroAddress(_creditManager)
{
creditManager = ICreditManagerV2(_creditManager); // F:[FA-1A]
underlying = ICreditManagerV2(_creditManager).underlying(); // F:[FA-1A]
wethAddress = ICreditManagerV2(_creditManager).wethAddress(); // F:[FA-1A]
Expand Down Expand Up @@ -329,13 +327,10 @@ contract CreditFacade is ICreditFacade, ReentrancyGuard {
uint256 skipTokenMask,
bool convertWETH,
MultiCall[] calldata calls
) external payable override nonReentrant {
) external payable override nonReentrant nonZeroAddress(to) {
// Checks that the CA exists to revert early for late liquidations and save gas
address creditAccount = _getCreditAccountOrRevert(borrower); // F:[FA-2]

// Checks that the to address is not zero
if (to == address(0)) revert ZeroAddressException(); // F:[FA-16A]

// Checks that the account hf < 1 and computes the totalValue
// before the multicall
(bool isLiquidatable, uint256 totalValue) = _isAccountLiquidatable(creditAccount); // F:[FA-14]
Expand Down Expand Up @@ -384,13 +379,10 @@ contract CreditFacade is ICreditFacade, ReentrancyGuard {
uint256 skipTokenMask,
bool convertWETH,
MultiCall[] calldata calls
) external payable override nonReentrant {
) external payable override nonReentrant nonZeroAddress(to) {
// Checks that the CA exists to revert early for late liquidations and save gas
address creditAccount = _getCreditAccountOrRevert(borrower);

// Checks that the to address is not zero
if (to == address(0)) revert ZeroAddressException();

// Checks that this Credit Facade is expired and reverts if not
if (!_isExpired()) {
revert CantLiquidateNonExpiredException(); // F: [FA-47,48]
Expand Down
68 changes: 30 additions & 38 deletions contracts/credit/CreditManager.sol
View file
Original file line number Diff line number Diff line change
Expand Up @@ -20,7 +20,7 @@ import {IWETHGateway} from "../interfaces/IWETHGateway.sol";
import {ICreditManagerV2, ClosureAction, CollateralTokenData} from "../interfaces/ICreditManagerV2.sol";
import {IAddressProvider} from "@gearbox-protocol/core-v2/contracts/interfaces/IAddressProvider.sol";
import {IPriceOracleV2} from "@gearbox-protocol/core-v2/contracts/interfaces/IPriceOracle.sol";
import {IPoolQuotaKeeper, QuotaUpdate, TokenLT, QuotaStatusChange} from "../interfaces/IPoolQuotaKeeper.sol";
import {IPoolQuotaKeeper, QuotaUpdate, TokenLT} from "../interfaces/IPoolQuotaKeeper.sol";
import {IVersion} from "@gearbox-protocol/core-v2/contracts/interfaces/IVersion.sol";

// CONSTANTS
Expand All @@ -35,6 +35,9 @@ import {
UNIVERSAL_CONTRACT
} from "@gearbox-protocol/core-v2/contracts/libraries/Constants.sol";

// EXCEPTIONS
import {TokenAlreadyAddedException, TokenNotAllowedException} from "../interfaces/IErrors.sol";

import "forge-std/console.sol";

uint256 constant ADDR_BIT_SIZE = 160;
Expand Down Expand Up @@ -192,7 +195,7 @@ contract CreditManager is ICreditManagerV2, ACLNonReentrantTrait {

/// @dev Constructor
/// @param _pool Address of the pool to borrow funds from
constructor(address _pool) ACLNonReentrantTrait(address(IPoolService(_pool).addressProvider())) {
constructor(address _pool) ACLNonReentrantTrait(address(IPool4626(_pool).addressProvider())) {
IAddressProvider addressProvider = IPoolService(_pool).addressProvider();

pool = _pool; // F:[CM-1]
Expand Down Expand Up @@ -325,7 +328,8 @@ contract CreditManager is ICreditManagerV2, ACLNonReentrantTrait {
if (supportsQuotas) {
TokenLT[] memory tokens = getLimitedTokens(creditAccount);

quotaInterest = cumulativeQuotaInterest[creditAccount];
// TODO: Check that it never breaks
quotaInterest = cumulativeQuotaInterest[creditAccount] - 1;

if (tokens.length > 0) {
quotaInterest += poolQuotaKeeper().closeCreditAccount(creditAccount, tokens); // F: [CMQ-6]
Expand Down Expand Up @@ -500,7 +504,7 @@ contract CreditManager is ICreditManagerV2, ACLNonReentrantTrait {
amountRepaid = _amountRepaid;
amountProfit = _amountProfit;

uint16 fee = slot1.feeInterest;
uint16 feeInterest = slot1.feeInterest;
uint256 quotaInterestAccrued = cumulativeQuotaInterest[creditAccount];

TokenLT[] memory tokens = getLimitedTokens(creditAccount);
Expand All @@ -509,19 +513,22 @@ contract CreditManager is ICreditManagerV2, ACLNonReentrantTrait {
}

if (quotaInterestAccrued > 2) {
uint256 quotaProfit = (quotaInterestAccrued * fee) / PERCENTAGE_FACTOR;
uint256 quotaProfit = (quotaInterestAccrued * feeInterest) / PERCENTAGE_FACTOR;

if (amountRepaid >= quotaInterestAccrued + quotaProfit) {
amountRepaid -= quotaInterestAccrued + quotaProfit; // F: [CMQ-5]
amountProfit += quotaProfit; // F: [CMQ-5]
cumulativeQuotaInterest[creditAccount] = 1; // F: [CMQ-5]
} else {
uint256 amountToPool = (amountRepaid * PERCENTAGE_FACTOR) / (PERCENTAGE_FACTOR + fee);
uint256 amountToPool = (amountRepaid * PERCENTAGE_FACTOR) / (PERCENTAGE_FACTOR + feeInterest);

amountProfit += amountRepaid - amountToPool; // F: [CMQ-4]
amountRepaid = 0; // F: [CMQ-4]

cumulativeQuotaInterest[creditAccount] = quotaInterestAccrued - amountToPool + 1; // F: [CMQ-4]
uint256 newCumulativeQuotaInterest = quotaInterestAccrued - amountToPool;

cumulativeQuotaInterest[creditAccount] =
newCumulativeQuotaInterest == 0 ? 1 : newCumulativeQuotaInterest; // F: [CMQ-4]
}
}
}
Expand Down Expand Up @@ -888,14 +895,16 @@ contract CreditManager is ICreditManagerV2, ACLNonReentrantTrait {

uint256 j;

while (tokenMask <= limitMask) {
if (limitMask & tokenMask != 0) {
(address token, uint16 lt) = collateralTokensByMask(tokenMask);
tokens[j] = TokenLT({token: token, lt: lt});
++j;
}
unchecked {
while (tokenMask <= limitMask) {
if (limitMask & tokenMask != 0) {
(address token, uint16 lt) = collateralTokensByMask(tokenMask);
tokens[j] = TokenLT({token: token, lt: lt});
++j;
}

tokenMask = tokenMask << 1;
tokenMask = tokenMask << 1;
}
}
}
}
Expand Down Expand Up @@ -1028,34 +1037,17 @@ contract CreditManager is ICreditManagerV2, ACLNonReentrantTrait {
override
creditFacadeOnly // F: [CMQ-3]
{
(uint256 caInterestChange, QuotaStatusChange[] memory statusChanges, bool statusWasChanged) =
poolQuotaKeeper().updateQuotas(creditAccount, quotaUpdates); // F: [CMQ-3]
(uint256 caInterestChange, uint256 enabledTokensMask) =
poolQuotaKeeper().updateQuotas(creditAccount, quotaUpdates, enabledTokensMap[creditAccount]); // F: [CMQ-3]

cumulativeQuotaInterest[creditAccount] += caInterestChange; // F: [CMQ-3]

if (statusWasChanged) {
uint256 len = quotaUpdates.length;
uint256 enabledTokensMask = enabledTokensMap[creditAccount];

for (uint256 i = 0; i < len;) {
if (statusChanges[i] == QuotaStatusChange.ZERO_TO_POSITIVE) {
enabledTokensMask |= tokenMasksMap(quotaUpdates[i].token); // F: [CMQ-3]
} else if (statusChanges[i] == QuotaStatusChange.POSITIVE_TO_ZERO) {
enabledTokensMask &= ~tokenMasksMap(quotaUpdates[i].token); // F: [CMQ-3]
}

unchecked {
++i;
}
}

uint256 totalTokensEnabled = _calcEnabledTokens(enabledTokensMask);
if (totalTokensEnabled > maxAllowedEnabledTokenLength) {
revert TooManyEnabledTokensException(); // F: [CMQ-11]
}

enabledTokensMap[creditAccount] = enabledTokensMask; // F: [CMQ-3]
uint256 totalTokensEnabled = _calcEnabledTokens(enabledTokensMask);
if (totalTokensEnabled > maxAllowedEnabledTokenLength) {
revert TooManyEnabledTokensException(); // F: [CMQ-11]
}

enabledTokensMap[creditAccount] = enabledTokensMask; // F: [CMQ-3]
}

/// @dev Checks if the contract is paused; if true, checks that the caller is emergency liquidator
Expand Down
Loading

0 comments on commit 5dd3b9f

Please sign in to comment.