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: fixes after ChainSecurity audit #150

Closed
wants to merge 3 commits into from
Closed

fix: fixes after ChainSecurity audit #150

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
dd6ea1d
fix: exclude underlying from enabled tokens count
lekhovitsky Oct 24, 2023
7abcaa7
fix: sanitize bot permissions
lekhovitsky Oct 24, 2023
8a7f19d
test: fix bot integration tests
lekhovitsky Oct 24, 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
3 changes: 3 additions & 0 deletions contracts/credit/CreditFacadeV3.sol
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand All @@ -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,
Expand Down
4 changes: 2 additions & 2 deletions contracts/credit/CreditManagerV3.sol
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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]
}

Expand Down
3 changes: 3 additions & 0 deletions contracts/interfaces/IExceptions.sol
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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 //
// ------ //
Expand Down
14 changes: 10 additions & 4 deletions contracts/test/integration/credit/Bots.int.sol
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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);
Expand All @@ -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});

Expand Down Expand Up @@ -127,15 +129,19 @@ 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),
abi.encodeCall(CreditManagerV3.setFlagFor, (creditAccount, BOT_PERMISSIONS_SET_FLAG, true))
);

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");

Expand Down
11 changes: 11 additions & 0 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 @@ -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);
Expand Down
2 changes: 1 addition & 1 deletion 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 @@ -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 {
Expand Down
Loading