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

Feat/fallback selector security check #85

Merged
merged 15 commits into from
Jun 2, 2024
Merged

Feat/fallback selector security check #85

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
15 commits
Select commit Hold shift + click to select a range
67864e1
chore: Fix indentation
Aboudjem May 29, 2024
d8e1252
🔒 Improve security on fallback handler installation process
Aboudjem May 29, 2024
2ced21b
Update gas report file name to uppercase + run prettier
Aboudjem May 29, 2024
dd22160
feat: Update gas report file name to uppercase
Aboudjem May 29, 2024
6d09a7b
chore: Update CI workflow to trigger only on pull requests
Aboudjem May 29, 2024
ccf87c4
Merge branch 'dev' into feat/fallback-selector-security-check
Aboudjem May 31, 2024
ae249db
chore: Improve fallback handler comments
Aboudjem May 31, 2024
c1d0b86
Merge branch 'feat/fallback-selector-security-check' of https://githu…
Aboudjem May 31, 2024
4acff06
refactor: Update gas report script paths and improve generation
Aboudjem Jun 1, 2024
9e25afd
refactor: Update gas report script paths and improve generation
Aboudjem Jun 1, 2024
c474f45
feat: Update gas report script paths and improve generation
Aboudjem Jun 1, 2024
4115305
Merge branch 'dev' into feat/fallback-selector-security-check
Aboudjem Jun 1, 2024
4e554b4
refactor: Update gas report script paths and improve generation
Aboudjem Jun 1, 2024
983093f
Merge branch 'dev' into feat/fallback-selector-security-check
Aboudjem Jun 2, 2024
3a03ad7
Merge branch 'dev' into feat/fallback-selector-security-check
Aboudjem Jun 2, 2024
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
2 changes: 1 addition & 1 deletion .github/gas_report.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -549,4 +549,4 @@
"RECEIVER_ACCESS": "N/A",
"GAS_USED": 184796
}
]
]
4 changes: 2 additions & 2 deletions .github/workflows/ci.yml
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,6 +1,6 @@
name: Unified CI Workflow

on: [push, pull_request]
on: pull_request

permissions: write-all

Expand Down Expand Up @@ -171,4 +171,4 @@ jobs:
console.log("Slither report is empty. No comment will be posted.");
return;
}
await script({ github, context, header, body })
await script({ github, context, header, body })
61 changes: 59 additions & 2 deletions GAS_REPORT.md
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,3 +1,60 @@
# Gas Report Comparison
| **Protocol** | **Actions / Function** | **Account Type** | **Is Deployed** | **With Paymaster?** | **Receiver Access** | **Gas Used** | **Gas Difference** |
|:------------:|:---------------------:|:----------------:|:--------------:|:-------------------:|:-------------------:|:------------:|:------------------:|

| **Protocol** | **Actions / Function** | **Account Type** | **Is Deployed** | **With Paymaster?** | **Receiver Access** | **Gas Used** | **Gas Difference** |
| :--------------------------------: | :------------------------------: | :--------------: | :-------------: | :-----------------: | :-----------------: | :----------: | :----------------: |
| ERC20 | transfer | EOA | False | False | 🧊 ColdAccess | 49921 | 0 |
| ERC20 | transfer | EOA | False | False | 🔥 WarmAccess | 25221 | 0 |
| ERC20 | transfer | Smart Account | True | False | 🧊 ColdAccess | 94767 | 0 |
| ERC20 | transfer | Smart Account | True | False | 🔥 WarmAccess | 74867 | 0 |
| ERC20 | transfer | Smart Account | False | True | 🧊 ColdAccess | 335883 | 0 |
| ERC20 | transfer | Smart Account | False | True | 🔥 WarmAccess | 315984 | 0 |
| ERC20 | transfer | Smart Account | False | False | 🧊 ColdAccess | 319073 | 0 |
| ERC20 | transfer | Smart Account | False | False | 🔥 WarmAccess | 299174 | 0 |
| ERC20 | transfer | Smart Account | False | False | 🧊 ColdAccess | 367178 | 0 |
| ERC20 | transfer | Smart Account | False | False | 🔥 WarmAccess | 347278 | 0 |
| ERC20 | transfer | Smart Account | True | True | 🧊 ColdAccess | 111262 | 0 |
| ERC20 | transfer | Smart Account | True | True | 🔥 WarmAccess | 91363 | 0 |
| ERC721 | transferFrom | EOA | False | False | 🧊 ColdAccess | 48483 | 0 |
| ERC721 | transferFrom | EOA | False | False | 🔥 WarmAccess | 28583 | 0 |
| ERC721 | transferFrom | Smart Account | True | False | 🧊 ColdAccess | 98254 | 0 |
| ERC721 | transferFrom | Smart Account | True | False | 🔥 WarmAccess | 78354 | 0 |
| ERC721 | transferFrom | Smart Account | False | True | 🧊 ColdAccess | 334585 | 0 |
| ERC721 | transferFrom | Smart Account | False | True | 🔥 WarmAccess | 314685 | 0 |
| ERC721 | transferFrom | Smart Account | False | False | 🧊 ColdAccess | 317777 | 0 |
| ERC721 | transferFrom | Smart Account | False | False | 🔥 WarmAccess | 297877 | 0 |
| ERC721 | transferFrom | Smart Account | False | False | 🧊 ColdAccess | 365881 | 0 |
| ERC721 | transferFrom | Smart Account | False | False | 🔥 WarmAccess | 345981 | 0 |
| ERC721 | transferFrom | Smart Account | True | True | 🧊 ColdAccess | 114777 | 0 |
| ERC721 | transferFrom | Smart Account | True | True | 🔥 WarmAccess | 94877 | 0 |
| ETH | transfer | EOA | False | False | 🧊 ColdAccess | 53073 | 0 |
| ETH | transfer | EOA | False | False | 🔥 WarmAccess | 28073 | 0 |
| ETH | call | EOA | False | False | 🧊 ColdAccess | 53201 | 0 |
| ETH | call | EOA | False | False | 🔥 WarmAccess | 28201 | 0 |
| ETH | send | EOA | False | False | 🧊 ColdAccess | 53201 | 0 |
| ETH | send | EOA | False | False | 🔥 WarmAccess | 28201 | 0 |
| ETH | transfer | Smart Account | True | False | 🧊 ColdAccess | 102616 | 0 |
| ETH | transfer | Smart Account | True | False | 🔥 WarmAccess | 77616 | 0 |
| ETH | transfer | Smart Account | False | True | 🧊 ColdAccess | 338898 | 0 |
| ETH | transfer | Smart Account | False | True | 🔥 WarmAccess | 313898 | 0 |
| ETH | transfer | Smart Account | False | False | 🧊 ColdAccess | 322110 | 0 |
| ETH | transfer | Smart Account | False | False | 🔥 WarmAccess | 297110 | 0 |
| ETH | transfer | Smart Account | False | False | 🧊 ColdAccess | 370215 | 0 |
| ETH | transfer | Smart Account | False | False | 🔥 WarmAccess | 345215 | 0 |
| ETH | transfer | Smart Account | True | True | 🧊 ColdAccess | 119101 | 0 |
| ETH | transfer | Smart Account | True | True | 🔥 WarmAccess | 94101 | 0 |
| UniswapV2 | swapExactETHForTokens | EOA | False | False | N/A | 149263 | 0 |
| UniswapV2 | swapExactETHForTokens | Smart Account | True | False | N/A | 199242 | 0 |
| UniswapV2 | swapExactETHForTokens | Smart Account | False | True | N/A | 435648 | 0 |
| UniswapV2 | swapExactETHForTokens | Smart Account | False | False | N/A | 418767 | 0 |
| UniswapV2 | swapExactETHForTokens | Smart Account | False | False | N/A | 466872 | 0 |
| UniswapV2 | swapExactETHForTokens | Smart Account | True | True | N/A | 215805 | 0 |
| UniswapV2 | swapExactTokensForTokens | EOA | False | False | N/A | 118252 | 0 |
| UniswapV2 | swapExactTokensForTokens | Smart Account | True | False | N/A | 168221 | 0 |
| UniswapV2 | swapExactTokensForTokens | Smart Account | False | True | N/A | 404616 | 0 |
| UniswapV2 | swapExactTokensForTokens | Smart Account | False | False | N/A | 387734 | 0 |
| UniswapV2 | approve+swapExactTokensForTokens | Smart Account | True | False | N/A | 200217 | 0 |
| UniswapV2 | approve+swapExactTokensForTokens | Smart Account | False | True | N/A | 436814 | 0 |
| UniswapV2 | approve+swapExactTokensForTokens | Smart Account | False | False | N/A | 419743 | 0 |
| UniswapV2 | approve+swapExactTokensForTokens | Smart Account | False | False | N/A | 467849 | 0 |
| UniswapV2 | swapExactTokensForTokens | Smart Account | True | True | N/A | 184796 | 0 |
| No differences found in gas usage. |
28 changes: 27 additions & 1 deletion contracts/base/ModuleManager.sol
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -213,11 +213,37 @@ contract ModuleManager is Storage, Receiver, IModuleManagerEventsAndErrors {
/// @param handler The address of the fallback handler to install.
/// @param params The initialization parameters including the selector and call type.
function _installFallbackHandler(address handler, bytes calldata params) internal virtual {
// Extract the function selector from the provided parameters.
bytes4 selector = bytes4(params[0:4]);

// Extract the call type from the provided parameters.
CallType calltype = CallType.wrap(bytes1(params[4]));

// Extract the initialization data from the provided parameters.
bytes memory initData = params[5:];
if (_isFallbackHandlerInstalled(selector)) revert FallbackAlreadyInstalledForSelector(selector);

// Revert if the selector is either `onInstall(bytes)` (0x6d61fe70) or `onUninstall(bytes)` (0x8a91b0e3).
// These selectors are explicitly forbidden to prevent security vulnerabilities.
// Allowing these selectors would enable unauthorized users to uninstall and reinstall critical modules.
// If a validator module is uninstalled and reinstalled without proper authorization, it can compromise
// the account's security and integrity. By restricting these selectors, we ensure that the fallback handler
// cannot be manipulated to disrupt the expected behavior and security of the account.
if (selector == bytes4(0x6d61fe70) || selector == bytes4(0x8a91b0e3)) {
revert FallbackSelectorForbidden();
}

// Revert if a fallback handler is already installed for the given selector.
// This check ensures that we do not overwrite an existing fallback handler, which could lead to unexpected behavior.
if (_isFallbackHandlerInstalled(selector)) {
revert FallbackAlreadyInstalledForSelector(selector);
}

// Store the fallback handler and its call type in the account storage.
// This maps the function selector to the specified fallback handler and call type.
_getAccountStorage().fallbacks[selector] = FallbackHandler(handler, calltype);

// Invoke the `onInstall` function of the fallback handler with the provided initialization data.
// This step allows the fallback handler to perform any necessary setup or initialization.
IFallback(handler).onInstall(initData);
}

Expand Down
3 changes: 3 additions & 0 deletions contracts/interfaces/base/IModuleManagerEventsAndErrors.sol
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -69,4 +69,7 @@ interface IModuleManagerEventsAndErrors {

/// @dev Thrown when no fallback handler is available for a given selector.
error MissingFallbackHandler(bytes4 selector);

/// @dev Thrown when there is an attempt to install a forbidden selector as a fallback handler.
error FallbackSelectorForbidden();
}
Loading
Loading