From dd6ea1db2e508938109ed31c81829f3cb7273806 Mon Sep 17 00:00:00 2001 From: Dima Lekhovitsky Date: Tue, 24 Oct 2023 14:10:05 +0300 Subject: [PATCH 1/3] fix: exclude underlying from enabled tokens count Now collateral check ensures that number of enabled _non-underlying_ tokens doesn't exceed the limit --- contracts/credit/CreditManagerV3.sol | 4 ++-- contracts/test/unit/credit/CreditManagerV3.unit.t.sol | 2 +- 2 files changed, 3 insertions(+), 3 deletions(-) diff --git a/contracts/credit/CreditManagerV3.sol b/contracts/credit/CreditManagerV3.sol index 45e33b8b..112a25d1 100644 --- a/contracts/credit/CreditManagerV3.sol +++ b/contracts/credit/CreditManagerV3.sol @@ -1145,9 +1145,9 @@ contract CreditManagerV3 is ICreditManagerV3, SanityCheckTrait, ReentrancyGuardT } /// @dev Saves `creditAccount`'s `enabledTokensMask` in the storage - /// @dev Ensures that the number of enabled tokens does not exceed `maxEnabledTokens` + /// @dev Ensures that the number of enabled tokens excluding underlying does not exceed `maxEnabledTokens` function _saveEnabledTokensMask(address creditAccount, uint256 enabledTokensMask) internal { - if (enabledTokensMask.calcEnabledTokens() > maxEnabledTokens) { + if (enabledTokensMask.disable(UNDERLYING_TOKEN_MASK).calcEnabledTokens() > maxEnabledTokens) { revert TooManyEnabledTokensException(); // U:[CM-37] } diff --git a/contracts/test/unit/credit/CreditManagerV3.unit.t.sol b/contracts/test/unit/credit/CreditManagerV3.unit.t.sol index a0694483..82b1547f 100644 --- a/contracts/test/unit/credit/CreditManagerV3.unit.t.sol +++ b/contracts/test/unit/credit/CreditManagerV3.unit.t.sol @@ -3089,7 +3089,7 @@ contract CreditManagerV3UnitTest is TestHelper, ICreditManagerV3Events, BalanceH vm.prank(CONFIGURATOR); creditManager.setMaxEnabledTokens(maxEnabledTokens); - if (mask.calcEnabledTokens() > maxEnabledTokens) { + if (mask.disable(UNDERLYING_TOKEN_MASK).calcEnabledTokens() > maxEnabledTokens) { vm.expectRevert(TooManyEnabledTokensException.selector); creditManager.saveEnabledTokensMask(creditAccount, mask); } else { From 7abcaa70f24f260a3dad3636c76692dd9b612618 Mon Sep 17 00:00:00 2001 From: Dima Lekhovitsky Date: Tue, 24 Oct 2023 15:10:38 +0300 Subject: [PATCH 2/3] fix: sanitize bot permissions --- contracts/credit/CreditFacadeV3.sol | 3 +++ contracts/interfaces/IExceptions.sol | 3 +++ contracts/test/unit/credit/CreditFacadeV3.unit.t.sol | 11 +++++++++++ 3 files changed, 17 insertions(+) diff --git a/contracts/credit/CreditFacadeV3.sol b/contracts/credit/CreditFacadeV3.sol index 37d245ce..978c30c6 100644 --- a/contracts/credit/CreditFacadeV3.sol +++ b/contracts/credit/CreditFacadeV3.sol @@ -473,6 +473,7 @@ contract CreditFacadeV3 is ICreditFacadeV3, ACLNonReentrantTrait { /// @param totalFundingAllowance Total amount of WETH available to bot for payments /// @param weeklyFundingAllowance Amount of WETH available to bot for payments weekly /// @dev Reverts if caller is not `creditAccount`'s owner + /// @dev Reverts if `permissions` has unexpected bits enabled /// @dev Reverts if account has more active bots than allowed after changing permissions // to prevent users from inflating liquidation gas costs /// @dev Changes account's `BOT_PERMISSIONS_SET_FLAG` in the credit manager if needed @@ -488,6 +489,8 @@ contract CreditFacadeV3 is ICreditFacadeV3, ACLNonReentrantTrait { creditAccountOwnerOnly(creditAccount) // U:[FA-5] nonReentrant // U:[FA-4] { + if (permissions & ~ALL_PERMISSIONS != 0) revert UnexpectedPermissionsException(); // U:[FA-41] + uint256 remainingBots = IBotListV3(botList).setBotPermissions({ creditManager: creditManager, creditAccount: creditAccount, diff --git a/contracts/interfaces/IExceptions.sol b/contracts/interfaces/IExceptions.sol index d452a846..aec6b7fc 100644 --- a/contracts/interfaces/IExceptions.sol +++ b/contracts/interfaces/IExceptions.sol @@ -184,6 +184,9 @@ error NoPermissionException(uint256 permission); /// @notice Thrown when user tries to approve more bots than allowed error TooManyApprovedBotsException(); +/// @notice Thrown when attempting to give a bot unexpected permissions +error UnexpectedPermissionsException(); + // ------ // // ACCESS // // ------ // diff --git a/contracts/test/unit/credit/CreditFacadeV3.unit.t.sol b/contracts/test/unit/credit/CreditFacadeV3.unit.t.sol index 2a70079b..4f6002b8 100644 --- a/contracts/test/unit/credit/CreditFacadeV3.unit.t.sol +++ b/contracts/test/unit/credit/CreditFacadeV3.unit.t.sol @@ -2083,6 +2083,17 @@ contract CreditFacadeV3UnitTest is TestHelper, BalanceHelper, ICreditFacadeV3Eve creditManagerMock.setBorrower(USER); + /// It reverts if passed unexpected permissions + vm.expectRevert(UnexpectedPermissionsException.selector); + vm.prank(USER); + creditFacade.setBotPermissions({ + creditAccount: creditAccount, + bot: bot, + permissions: type(uint192).max, + totalFundingAllowance: 2, + weeklyFundingAllowance: 3 + }); + creditManagerMock.setFlagFor({creditAccount: creditAccount, flag: BOT_PERMISSIONS_SET_FLAG, value: false}); botListMock.setBotPermissionsReturn(1); From 8a7f19d4556d7cbf7364b329b244762f18506773 Mon Sep 17 00:00:00 2001 From: Dima Lekhovitsky Date: Tue, 24 Oct 2023 16:01:46 +0300 Subject: [PATCH 3/3] test: fix bot integration tests --- contracts/test/integration/credit/Bots.int.sol | 14 ++++++++++---- 1 file changed, 10 insertions(+), 4 deletions(-) diff --git a/contracts/test/integration/credit/Bots.int.sol b/contracts/test/integration/credit/Bots.int.sol index 876566de..e65e3d53 100644 --- a/contracts/test/integration/credit/Bots.int.sol +++ b/contracts/test/integration/credit/Bots.int.sol @@ -58,7 +58,7 @@ contract BotsIntegrationTest is IntegrationTestHelper, ICreditFacadeV3Events { vm.prank(address(creditFacade)); botList.setBotPermissions( - address(creditManager), creditAccount, bot, type(uint192).max, uint72(1 ether), uint72(1 ether / 10) + address(creditManager), creditAccount, bot, uint192(ALL_PERMISSIONS), uint72(1 ether), uint72(1 ether / 10) ); vm.expectRevert(NotApprovedBotException.selector); @@ -78,7 +78,9 @@ contract BotsIntegrationTest is IntegrationTestHelper, ICreditFacadeV3Events { botList.setBotSpecialPermissions(address(creditManager), address(bot), 0); vm.prank(USER); - creditFacade.setBotPermissions(creditAccount, bot, type(uint192).max, uint72(1 ether), uint72(1 ether / 10)); + creditFacade.setBotPermissions( + creditAccount, bot, uint192(ALL_PERMISSIONS), uint72(1 ether), uint72(1 ether / 10) + ); botList.getBotStatus({creditManager: address(creditManager), creditAccount: creditAccount, bot: bot}); @@ -127,7 +129,9 @@ contract BotsIntegrationTest is IntegrationTestHelper, ICreditFacadeV3Events { vm.expectRevert(CallerNotCreditAccountOwnerException.selector); vm.prank(FRIEND); - creditFacade.setBotPermissions(creditAccount, bot, type(uint192).max, uint72(1 ether), uint72(1 ether / 10)); + creditFacade.setBotPermissions( + creditAccount, bot, uint192(ALL_PERMISSIONS), uint72(1 ether), uint72(1 ether / 10) + ); vm.expectCall( address(creditManager), @@ -135,7 +139,9 @@ contract BotsIntegrationTest is IntegrationTestHelper, ICreditFacadeV3Events { ); vm.prank(USER); - creditFacade.setBotPermissions(creditAccount, bot, type(uint192).max, uint72(1 ether), uint72(1 ether / 10)); + creditFacade.setBotPermissions( + creditAccount, bot, uint192(ALL_PERMISSIONS), uint72(1 ether), uint72(1 ether / 10) + ); assertTrue(creditManager.flagsOf(creditAccount) & BOT_PERMISSIONS_SET_FLAG > 0, "Flag was not set");