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.

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

feat: add erc20 token limit module #184

Merged
merged 6 commits into from
Sep 25, 2024
Merged

feat: add erc20 token limit module #184

merged 6 commits into from
Sep 25, 2024

Conversation

fangting-alchemy
Copy link
Collaborator

Motivation

Account holders may want to set ERC20 token spend limit to certain validations.

Solution

Create an ERC20 token limit module to offer hooks associated with validations that works with both UO and runtime call paths. See code doc for features and restrictions.

Copy link

octane-security-app-dev bot commented Sep 20, 2024

Summary by Octane

New Contracts

  • ERC20TokenLimitModule.sol: ERC20 Token Limit Module enforces spend limits on ERC20 tokens during validation, allowing only specific functions (transfer, approve) and supporting native execution functions like execute and executeBatch.

Updated Contracts

No contracts were updated in this PR.


🔗 Commit Hash: e4bce76

Copy link

github-actions bot commented Sep 20, 2024

Contract sizes:

 | Contract                     | Size (B) | Margin (B) |
 |------------------------------|----------|------------|
 | AccountFactory               |    4,169 |     20,407 |
+| ERC20TokenLimitModule        |    4,138 |     20,438 |
 | ModularAccount               |   25,983 |     -1,407 |
 | SemiModularAccount           |   26,487 |     -1,911 |
 | SingleSignerValidationModule |    3,444 |     21,132 |
 | TokenReceiverModule          |    2,189 |     22,387 |

Code coverage:

File % Lines % Statements % Branches % Funcs
src/account/AccountExecutor.sol 75.00% (3/4) 75.00% (3/4) 0.00% (0/1) 100.00% (1/1)
src/account/AccountStorageInitializable.sol 84.21% (16/19) 84.62% (22/26) 60.00% (3/5) 100.00% (2/2)
src/account/BaseAccount.sol 83.33% (5/6) 80.00% (4/5) 50.00% (1/2) 100.00% (2/2)
src/account/ModularAccount.sol 92.50% (222/240) 93.15% (299/321) 80.00% (32/40) 97.44% (38/39)
src/account/ModularAccountView.sol 96.55% (28/29) 95.24% (40/42) 100.00% (2/2) 100.00% (2/2)
src/account/ModuleManagerInternals.sol 84.92% (107/126) 84.62% (143/169) 42.86% (6/14) 100.00% (11/11)
src/account/SemiModularAccount.sol 0.00% (0/50) 0.00% (0/66) 0.00% (0/9) 0.00% (0/17)
src/factory/AccountFactory.sol 34.48% (10/29) 35.00% (14/40) 50.00% (1/2) 18.18% (2/11)
src/libraries/HookConfigLib.sol 47.06% (8/17) 65.62% (21/32) 100.00% (0/0) 66.67% (8/12)
src/libraries/KnownSelectorsLib.sol 100.00% (27/27) 100.00% (60/60) 100.00% (0/0) 100.00% (3/3)
src/libraries/ModuleEntityLib.sol 62.50% (5/8) 45.00% (9/20) 100.00% (0/0) 50.00% (3/6)
src/libraries/SparseCalldataSegmentLib.sol 100.00% (23/23) 100.00% (29/29) 100.00% (5/5) 100.00% (6/6)
src/libraries/ValidationConfigLib.sol 44.44% (8/18) 52.63% (20/38) 100.00% (0/0) 61.54% (8/13)
src/modules/BaseModule.sol 100.00% (11/11) 100.00% (15/15) 100.00% (1/1) 100.00% (2/2)
src/modules/ERC20TokenLimitModule.sol 70.27% (26/37) 76.92% (40/52) 70.00% (7/10) 55.56% (5/9)
src/modules/ModuleEIP712.sol 100.00% (1/1) 100.00% (2/2) 100.00% (0/0) 100.00% (1/1)
src/modules/ReplaySafeWrapper.sol 100.00% (6/6) 100.00% (7/7) 100.00% (0/0) 100.00% (2/2)
src/modules/TokenReceiverModule.sol 69.23% (9/13) 69.23% (9/13) 100.00% (0/0) 28.57% (2/7)
src/modules/validation/SingleSignerValidationModule.sol 90.48% (19/21) 91.30% (21/23) 100.00% (3/3) 88.89% (8/9)
Total 77.96% (534/685) 78.63% (758/964) 64.89% (61/94) 68.39% (106/155)

This comment was marked as spam.

Copy link
Contributor

@adamegyed adamegyed left a comment

Choose a reason for hiding this comment

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

Thanks for working on this! Had some comments.

src/modules/ERC20TokenLimitModule.sol Outdated Show resolved Hide resolved
src/modules/ERC20TokenLimitModule.sol Outdated Show resolved Hide resolved
src/modules/ERC20TokenLimitModule.sol Outdated Show resolved Hide resolved
src/modules/ERC20TokenLimitModule.sol Outdated Show resolved Hide resolved
src/modules/ERC20TokenLimitModule.sol Outdated Show resolved Hide resolved
src/modules/ERC20TokenLimitModule.sol Show resolved Hide resolved
src/modules/ERC20TokenLimitModule.sol Show resolved Hide resolved
src/modules/ERC20TokenLimitModule.sol Outdated Show resolved Hide resolved
src/modules/ERC20TokenLimitModule.sol Outdated Show resolved Hide resolved
src/modules/ERC20TokenLimitModule.sol Outdated Show resolved Hide resolved
Copy link
Contributor

@adamegyed adamegyed left a comment

Choose a reason for hiding this comment

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

Looks good! Had just small comment and unused import nits.

import {UserOperationLib} from "@eth-infinitism/account-abstraction/core/UserOperationLib.sol";
import {PackedUserOperation} from "@eth-infinitism/account-abstraction/interfaces/PackedUserOperation.sol";

import {SetValue} from "@erc6900/modular-account-libs/libraries/AssociatedLinkedListSetLib.sol";
Copy link
Contributor

Choose a reason for hiding this comment

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

With the refactor, this import and the _toSetValue internal function are no longer used.


import {SetValue} from "@erc6900/modular-account-libs/libraries/AssociatedLinkedListSetLib.sol";
import {IERC20} from "@openzeppelin/contracts/token/ERC20/IERC20.sol";
import {EnumerableSet} from "@openzeppelin/contracts/utils/structs/EnumerableSet.sol";
Copy link
Contributor

Choose a reason for hiding this comment

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

The EnumerableSet import and the using EnumerableSet for EnumerableSet.AddressSet; statement are both unused now and can be removed. I think the linter sees the using statement and thinks it is used, so it misses this case.

return "";
}

/// @inheritdoc IModule
Copy link
Contributor

Choose a reason for hiding this comment

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

Similar to the natspec comment requests for other modules, could you add the encoding format of data to natpspec as an @dev comment? This should help by making it show up in docs and on explorers.

@fangting-alchemy fangting-alchemy merged commit 5b4c786 into develop Sep 25, 2024
6 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants