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

fix: add more explicit account existence checks #158

Merged
merged 2 commits into from
Nov 24, 2023
Merged

fix: add more explicit account existence checks #158

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
5fc7667
fix: add more explicit account existence checks
lekhovitsky Nov 22, 2023
a1601b3
fix: cleaner liquidation + missing facade tests
lekhovitsky Nov 23, 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
13 changes: 8 additions & 5 deletions contracts/credit/CreditFacadeV3.sol
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -234,7 +234,7 @@ contract CreditFacadeV3 is ICreditFacadeV3, ACLNonReentrantTrait {
/// - Erases all bots permissions
/// @param creditAccount Account to close
/// @param calls List of calls to perform before closing the account
/// @dev Reverts if caller is not `creditAccount`'s owner
/// @dev Reverts if `creditAccount` is not opened in connected credit manager by caller
/// @dev Reverts if facade is paused
/// @dev Reverts if account has enabled tokens after executing `calls`
/// @dev Reverts if account's debt is not zero after executing `calls`
Expand Down Expand Up @@ -296,7 +296,7 @@ contract CreditFacadeV3 is ICreditFacadeV3, ACLNonReentrantTrait {
whenNotPausedOrEmergency // U:[FA-2,12]
nonReentrant // U:[FA-4]
{
address borrower = _getBorrowerOrRevert(creditAccount); // U:[FA-5]
Copy link
Collaborator Author

@lekhovitsky lekhovitsky Nov 23, 2023 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

borrower is removed from LiquidateCreditAccount event, and the check is now performed in calcDebtAndCollateral (additional safe-guard is require(debt != 0) below).

_getBorrowerOrRevert(creditAccount); // U:[FA-5]

uint256 skipCalls = _applyOnDemandPriceUpdates(calls);

Expand Down Expand Up @@ -343,7 +343,7 @@ contract CreditFacadeV3 is ICreditFacadeV3, ACLNonReentrantTrait {
isExpired: isExpired
}); // U:[FA-16]

emit LiquidateCreditAccount(creditAccount, borrower, msg.sender, to, remainingFunds); // U:[FA-14,16,17]
emit LiquidateCreditAccount(creditAccount, msg.sender, to, remainingFunds); // U:[FA-14,16,17]

if (reportedLoss != 0) {
maxDebtPerBlockMultiplier = 0; // U:[FA-17]
Expand All @@ -364,7 +364,7 @@ contract CreditFacadeV3 is ICreditFacadeV3, ACLNonReentrantTrait {
/// - Runs the collateral check
/// @param creditAccount Account to perform the calls on
/// @param calls List of calls to perform
/// @dev Reverts if caller is not `creditAccount`'s owner
/// @dev Reverts if `creditAccount` is not opened in connected credit manager by caller
/// @dev Reverts if credit facade is paused or expired
function multicall(address creditAccount, MultiCall[] calldata calls)
external
Expand All @@ -386,6 +386,7 @@ contract CreditFacadeV3 is ICreditFacadeV3, ACLNonReentrantTrait {
/// @param creditAccount Account to perform the calls on
/// @param calls List of calls to perform
/// @dev Reverts if credit facade is paused or expired
/// @dev Reverts if `creditAccount` is not opened in connected credit manager
/// @dev Reverts if calling bot is forbidden or has no permissions to manage `creditAccount`
function botMulticall(address creditAccount, MultiCall[] calldata calls)
external
Expand All @@ -394,6 +395,8 @@ contract CreditFacadeV3 is ICreditFacadeV3, ACLNonReentrantTrait {
whenNotExpired // U:[FA-3]
nonReentrant // U:[FA-4]
{
_getBorrowerOrRevert(creditAccount); // U:[FA-5]
Copy link
Collaborator Author

@lekhovitsky lekhovitsky Nov 23, 2023 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Absence of this could have been a problem with addition of special bots (for normal bots, permissions != 0 implies account existence).


(uint256 botPermissions, bool forbidden, bool hasSpecialPermissions) = IBotListV3(botList).getBotStatus({
bot: msg.sender,
creditManager: creditManager,
Expand All @@ -414,7 +417,7 @@ contract CreditFacadeV3 is ICreditFacadeV3, ACLNonReentrantTrait {
/// @param creditAccount Account to set permissions for
/// @param bot Bot to set permissions for
/// @param permissions A bit mask encoding bot permissions
/// @dev Reverts if caller is not `creditAccount`'s owner
/// @dev Reverts if `creditAccount` is not opened in connected credit manager by caller
/// @dev Reverts if `permissions` has unexpected bits enabled
/// @dev Reverts if account has more active bots than allowed after changing permissions
/// @dev Changes account's `BOT_PERMISSIONS_SET_FLAG` in the credit manager if needed
Expand Down
11 changes: 8 additions & 3 deletions contracts/credit/CreditManagerV3.sol
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -211,7 +211,7 @@ contract CreditManagerV3 is ICreditManagerV3, SanityCheckTrait, ReentrancyGuardT

/// @notice Closes a credit account
/// @param creditAccount Account to close
/// @custom:expects Account is opened in this credit manager
/// @custom:expects Credit facade ensures that `creditAccount` is opened in this credit manager
function closeCreditAccount(address creditAccount)
external
override
Expand Down Expand Up @@ -249,7 +249,7 @@ contract CreditManagerV3 is ICreditManagerV3, SanityCheckTrait, ReentrancyGuardT
/// @param to Address to transfer underlying left after liquidation
/// @return remainingFunds Total value of assets left on the account after liquidation
/// @return loss Loss incurred on liquidation
/// @custom:expects Account is opened in this credit manager
/// @custom:expects Credit facade ensures that `creditAccount` is opened in this credit manager
/// @custom:expects `collateralDebtData` is a result of `calcDebtAndCollateral` in `DEBT_COLLATERAL` mode
function liquidateCreditAccount(
address creditAccount,
Expand Down Expand Up @@ -630,7 +630,10 @@ contract CreditManagerV3 is ICreditManagerV3, SanityCheckTrait, ReentrancyGuardT
/// @notice Whether `creditAccount`'s health factor is below `minHealthFactor`
/// @param creditAccount Credit account to check
/// @param minHealthFactor Health factor threshold in bps
/// @dev Reverts if account is not opened in this credit manager
function isLiquidatable(address creditAccount, uint16 minHealthFactor) external view override returns (bool) {
getBorrowerOrRevert(creditAccount); // U:[CM-17]
Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Explicitly reverting is better than relying on some underflow later.


uint256[] memory collateralHints;
CollateralDebtData memory cdd = _calcDebtAndCollateral({
creditAccount: creditAccount,
Expand All @@ -648,6 +651,7 @@ contract CreditManagerV3 is ICreditManagerV3, SanityCheckTrait, ReentrancyGuardT
/// @param creditAccount Credit account to return data for
/// @param task Calculation mode, see `CollateralCalcTask` for details, can't be `FULL_COLLATERAL_CHECK_LAZY`
/// @return cdd A struct with debt and collateral data
/// @dev Reverts if account is not opened in this credit manager
/// @custom:invariant From `creditAccountInfo[creditAccount].debt == 0` it follows that `cdd.totalDebtUSD == 0`
function calcDebtAndCollateral(address creditAccount, CollateralCalcTask task)
external
Expand All @@ -660,12 +664,13 @@ contract CreditManagerV3 is ICreditManagerV3, SanityCheckTrait, ReentrancyGuardT
}

bool useSafePrices;

if (task == CollateralCalcTask.DEBT_COLLATERAL_SAFE_PRICES) {
task = CollateralCalcTask.DEBT_COLLATERAL;
useSafePrices = true;
}

getBorrowerOrRevert(creditAccount); // U:[CM-17]
Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Explicitly reverting is better than relying on some underflow later.


uint256[] memory collateralHints;
cdd = _calcDebtAndCollateral({
creditAccount: creditAccount,
Expand Down
6 changes: 1 addition & 5 deletions contracts/interfaces/ICreditFacadeV3.sol
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -51,11 +51,7 @@ interface ICreditFacadeV3Events {

/// @notice Emitted when account is liquidated
event LiquidateCreditAccount(
address indexed creditAccount,
address indexed borrower,
address indexed liquidator,
address to,
uint256 remainingFunds
address indexed creditAccount, address indexed liquidator, address to, uint256 remainingFunds
);

/// @notice Emitted when account's debt is increased
Expand Down
2 changes: 1 addition & 1 deletion contracts/test/integration/credit/LiquidateCreditAccount.int.t.sol
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -110,7 +110,7 @@ contract LiquidateCreditAccountIntegrationTest is IntegrationTestHelper, ICredit
// );

vm.expectEmit(true, true, true, true);
emit LiquidateCreditAccount(creditAccount, USER, LIQUIDATOR, FRIEND, 0);
emit LiquidateCreditAccount(creditAccount, LIQUIDATOR, FRIEND, 0);

vm.prank(LIQUIDATOR);
creditFacade.liquidateCreditAccount(creditAccount, FRIEND, calls);
Expand Down
14 changes: 9 additions & 5 deletions contracts/test/unit/credit/CreditFacadeV3.unit.t.sol
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -252,8 +252,8 @@ contract CreditFacadeV3UnitTest is TestHelper, BalanceHelper, ICreditFacadeV3Eve
creditFacade.setBotPermissions({creditAccount: DUMB_ADDRESS, bot: DUMB_ADDRESS, permissions: 0});
}

/// @dev U:[FA-5]: borrower related functions revert if called not by borrower
function test_U_FA_05_borrower_related_functions_revert_if_called_not_by_borrower() public notExpirableCase {
/// @dev U:[FA-5]: Account management functions revert if account does not exist
function test_U_FA_05_account_management_functions_revert_if_account_does_not_exist() public notExpirableCase {
vm.expectRevert(CreditAccountDoesNotExistException.selector);
creditFacade.closeCreditAccount({creditAccount: DUMB_ADDRESS, calls: new MultiCall[](0)});

Expand All @@ -263,6 +263,9 @@ contract CreditFacadeV3UnitTest is TestHelper, BalanceHelper, ICreditFacadeV3Eve
vm.expectRevert(CreditAccountDoesNotExistException.selector);
creditFacade.multicall({creditAccount: DUMB_ADDRESS, calls: new MultiCall[](0)});

vm.expectRevert(CreditAccountDoesNotExistException.selector);
creditFacade.botMulticall({creditAccount: DUMB_ADDRESS, calls: new MultiCall[](0)});

vm.expectRevert(CreditAccountDoesNotExistException.selector);
creditFacade.setBotPermissions({creditAccount: DUMB_ADDRESS, bot: DUMB_ADDRESS, permissions: 0});
}
Expand Down Expand Up @@ -581,7 +584,7 @@ contract CreditFacadeV3UnitTest is TestHelper, BalanceHelper, ICreditFacadeV3Eve
// : ClosureAction.LIQUIDATE_ACCOUNT;

// vm.expectEmit(true, true, true, true);
// emit LiquidateCreditAccount(creditAccount, USER, LIQUIDATOR, FRIEND, closeAction, 0);
// emit LiquidateCreditAccount(creditAccount, LIQUIDATOR, FRIEND, closeAction, 0);

// vm.expectCall(
// address(creditManagerMock),
Expand Down Expand Up @@ -772,7 +775,7 @@ contract CreditFacadeV3UnitTest is TestHelper, BalanceHelper, ICreditFacadeV3Eve
// creditManagerMock.setCloseCreditAccountReturns(1_000, 0);

// vm.expectEmit(true, true, true, true);
// emit LiquidateCreditAccount(creditAccount, USER, LIQUIDATOR, FRIEND, ClosureAction.LIQUIDATE_ACCOUNT, 1_000);
// emit LiquidateCreditAccount(creditAccount, LIQUIDATOR, FRIEND, ClosureAction.LIQUIDATE_ACCOUNT, 1_000);

// vm.prank(LIQUIDATOR);
// creditFacade.liquidateCreditAccount({
Expand Down Expand Up @@ -834,7 +837,7 @@ contract CreditFacadeV3UnitTest is TestHelper, BalanceHelper, ICreditFacadeV3Eve
// );

// vm.expectEmit(true, true, true, true);
// emit LiquidateCreditAccount(creditAccount, USER, LIQUIDATOR, FRIEND, ClosureAction.LIQUIDATE_ACCOUNT, 1_000);
// emit LiquidateCreditAccount(creditAccount, LIQUIDATOR, FRIEND, ClosureAction.LIQUIDATE_ACCOUNT, 1_000);

// vm.prank(LIQUIDATOR);
// creditFacade.liquidateCreditAccount({
Expand Down Expand Up @@ -912,6 +915,7 @@ contract CreditFacadeV3UnitTest is TestHelper, BalanceHelper, ICreditFacadeV3Eve
/// @dev U:[FA-19]: botMulticall reverts if 1. bot permissions flag is false 2. no permissions are set 3. bot is forbidden
function test_U_FA_19_botMulticall_reverts_for_invalid_bots() public notExpirableCase {
address creditAccount = DUMB_ADDRESS;
creditManagerMock.setBorrower(USER);
MultiCall[] memory calls;

creditManagerMock.setFlagFor(creditAccount, BOT_PERMISSIONS_SET_FLAG, true);
Expand Down
18 changes: 18 additions & 0 deletions contracts/test/unit/credit/CreditManagerV3.unit.t.sol
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1591,6 +1591,24 @@ contract CreditManagerV3UnitTest is TestHelper, ICreditManagerV3Events, BalanceH
assertEq(returnValue, expectedReturnValue, "Incorrect return value");
}

/// @dev U:[CM-17]: `calcDebtAndCollateral` reverts if account does not exist
function test_U_CM_17_calcDebtAndCollateral_reverts_if_account_does_not_exist() public creditManagerTest {
vm.expectRevert(CreditAccountDoesNotExistException.selector);
creditManager.calcDebtAndCollateral(DUMB_ADDRESS, CollateralCalcTask.GENERIC_PARAMS);

vm.expectRevert(CreditAccountDoesNotExistException.selector);
creditManager.calcDebtAndCollateral(DUMB_ADDRESS, CollateralCalcTask.DEBT_ONLY);

vm.expectRevert(CreditAccountDoesNotExistException.selector);
creditManager.calcDebtAndCollateral(DUMB_ADDRESS, CollateralCalcTask.DEBT_COLLATERAL);

vm.expectRevert(CreditAccountDoesNotExistException.selector);
creditManager.calcDebtAndCollateral(DUMB_ADDRESS, CollateralCalcTask.DEBT_COLLATERAL_SAFE_PRICES);

vm.expectRevert(CreditAccountDoesNotExistException.selector);
creditManager.isLiquidatable(DUMB_ADDRESS, PERCENTAGE_FACTOR);
}

//
//
// FULL COLLATERAL CHECK
Expand Down
Loading