feat: add whitelisting to erc20 custody v2 #296

skosito · 2024-08-07T23:10:23Z

add whitelisting to erc20custody
protect erc20 custody methods that accept token as param
in 2 methods sending tokens from gateway to custody, adds this check

Summary by CodeRabbit

New Features
- Introduced WHITELISTER_ROLE for managing token whitelisting.
- Added functions for whitelisting and unwhitelisting tokens, enhancing token management.
- New events Whitelisted and Unwhitelisted for tracking token status changes.
- Improved withdrawal functions to prevent operations on unapproved tokens.
Bug Fixes
- Implemented error handling for unauthorized token interactions, including NotWhitelisted and NotWhitelistedInCustody.
Tests
- Added tests for whitelisting functions and withdrawal operations to ensure proper functionality and security measures.

coderabbitai · 2024-08-07T23:10:32Z

Caution

Review failed

The pull request is closed.

Walkthrough

The changes introduce significant enhancements to several smart contracts, focusing on role management, token whitelisting, and event handling. New functions and events improve security and usability by ensuring that only approved tokens can be managed and interacted with. The updates also refine error handling, making it clearer when operations fail due to access control issues. Overall, these modifications aim to bolster the functionality and robustness of the contracts in managing ERC20 tokens.

Changes

Files	Change Summary
`v2/src/evm/ERC20Custody.sol`, `v2/pkg/erc20custody.sol/erc20custody.go`	Added `WHITELISTER_ROLE`, functions for managing a whitelist, and events for whitelisting. Updated withdrawal functions to check whitelist status.
`v2/src/evm/GatewayEVM.sol`, `v2/test/GatewayEVM.t.sol`	Implemented checks for token whitelisting before custody transfers. Added new tests for deposit functionality to enforce whitelisting rules.
`v2/src/evm/interfaces/IERC20Custody.sol`, `v2/typechain-types/IERC20Custody.ts`	Enhanced interface with new events and error definitions related to whitelisting. Introduced functions for checking and managing token whitelist status.
`v2/pkg/igatewayevm.sol/igatewayevm.go`	Updated to include new error types and improve event handling capabilities.
`v2/pkg/ierc20custody.sol/ierc20custodyevents.go`, `v2/pkg/ierc20custody.sol/ierc20custodyerrors.go`	Introduced new events and errors to enhance the contract's functionality and error feedback.
`v2/test/ERC20Custody.t.sol`	Added tests for new error conditions and whitelisting logic.

Sequence Diagram(s)

sequenceDiagram
    participant User
    participant ERC20Custody
    participant Token

    User->>ERC20Custody: Request to whitelist token
    ERC20Custody->>Token: Verify token validity
    Token-->>ERC20Custody: Token is valid
    ERC20Custody->>ERC20Custody: Update whitelist
    ERC20Custody-->>User: Token whitelisted

    User->>ERC20Custody: Attempt to withdraw token
    ERC20Custody->>ERC20Custody: Check if token is whitelisted
    alt Token is whitelisted
        ERC20Custody-->>User: Withdrawal successful
    else Token is not whitelisted
        ERC20Custody-->>User: NotWhitelisted error
    end

🐰 In the garden of code, where the tokens play,
A rabbit hops by, with a wish for the day.
"Whitelisted tokens, oh what a delight!
With access so clear, everything feels right!
For every withdrawal, a check let it be,
A secure little world, just for you and me!" 🥕

Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media?

Share

Tips

Chat

There are 3 ways to chat with CodeRabbit:

Review comments: Directly reply to a review comment made by CodeRabbit. Example:
- I pushed a fix in commit <commit_id>.
- Generate unit testing code for this file.
- Open a follow-up GitHub issue for this discussion.
Files and specific lines of code (under the "Files changed" tab): Tag @coderabbitai in a new review comment at the desired location with your query. Examples:
- @coderabbitai generate unit testing code for this file.
- @coderabbitai modularize this function.
PR comments: Tag @coderabbitai in a new PR comment to ask questions about the PR branch. For the best results, please provide a very specific query, as very limited context is provided in this mode. Examples:
- @coderabbitai generate interesting stats about this repository and render them as a table.
- @coderabbitai show all the console.log statements in this repository.
- @coderabbitai read src/utils.ts and generate unit testing code.
- @coderabbitai read the files in the src/scheduler package and generate a class diagram using mermaid and a README in the markdown format.
- @coderabbitai help me debug CodeRabbit configuration file.

Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments.

CodeRabbit Commands (invoked as PR comments)

@coderabbitai pause to pause the reviews on a PR.
@coderabbitai resume to resume the paused reviews.
@coderabbitai review to trigger an incremental review. This is useful when automatic reviews are disabled for the repository.
@coderabbitai full review to do a full review from scratch and review all the files again.
@coderabbitai summary to regenerate the summary of the PR.
@coderabbitai resolve resolve all the CodeRabbit review comments.
@coderabbitai configuration to show the current CodeRabbit configuration for the repository.
@coderabbitai help to get help.

Additionally, you can add @coderabbitai ignore anywhere in the PR description to prevent this PR from being reviewed.

CodeRabbit Configuration File (`.coderabbit.yaml`)

You can programmatically configure CodeRabbit by adding a .coderabbit.yaml file to the root of your repository.
Please see the configuration documentation for more information.
If your editor has YAML language server enabled, you can add the path at the top of this file to enable auto-completion and validation: # yaml-language-server: $schema=https://coderabbit.ai/integrations/schema.v2.json

Documentation and Community

Visit our Documentation for detailed information on how to use CodeRabbit.
Join our Discord Community to get help, request features, and share feedback.
Follow us on X/Twitter for updates and announcements.

codecov-commenter · 2024-08-07T23:14:53Z

Codecov Report

Attention: Patch coverage is 92.30769% with 1 line in your changes missing coverage. Please review.

Project coverage is 83.09%. Comparing base (c284f9b) to head (ec450b2).

Files	Patch %	Lines
v2/src/evm/GatewayEVM.sol	50.00%	1 Missing ⚠️

Additional details and impacted files

@@                   Coverage Diff                   @@
##           improve-v2-coverage     #296      +/-   ##
=======================================================
+ Coverage                82.65%   83.09%   +0.44%     
=======================================================
  Files                        7        7              
  Lines                      271      284      +13     
  Branches                    85       92       +7     
=======================================================
+ Hits                       224      236      +12     
- Misses                      47       48       +1

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

v2/src/evm/ERC20Custody.sol

fbac · 2024-08-08T08:00:08Z

v2/src/evm/ERC20Custody.sol

+    function whitelist(address token) external onlyRole(WHITELISTER_ROLE) {
+        whitelisted[token] = true;
+        emit Whitelisted(token);
+    }


Before whitelisting:

Check address != address(0).

Check address is a contract (address.code > 0)

Also, ideally we would check as well address.totalSupply() or any other IERC20 interface method exists.

That way we make sure whitelisted only contain valid ERC20 contracts.

fixed address point

regarding erc20 interface, there is separate issue for that, lets handle it on codebase level: #239

not sure what is the best way for that, i dont think that check if single method exists is precise enough in theory because we want to check for whole interface, which is more something like supportsInterface which i think exists on some standrads

Sounds good, after some investigation it seems like the best idea: supportsInterface(0x36372b07) should be it.

lumtis

LGTM

skosito added 17 commits August 7, 2024 15:39

split custody tests into separate file and add pause tests for custody

3fe0d86

add tests for non native connector max supply

9b0385e

add more connector tests

8c3e4f6

cleanup

f11f4be

add gateway set custody connector tests

c730f4c

test for zero addresses

99328df

zero address test

91c5227

add zeta non eth deposit test

40bf55c

add basic checks and tests for gatewayzevm

761ba15

tests for no balance

6c73552

add missing zrc20 tests

4f042cc

yarn generate

c698398

Merge branch 'main' into improve-v2-coverage

c284f9b

add whitelist to erc20 custody

c3e2dca

add erc20custody interface and cleanup interfaces

5777543

add whitelist checks when sending from gateway to custody

7ac1ef9

generate

c45cd1f

skosito requested review from andresaiello, brewmaster012, lumtis, CharlieMc0, fadeev and fbac as code owners August 7, 2024 23:10

skosito linked an issue Aug 7, 2024 that may be closed by this pull request

Add whitelist mechanism for the new ERC20CustodyContract #295

Closed

fbac reviewed Aug 8, 2024

View reviewed changes

lumtis approved these changes Aug 8, 2024

View reviewed changes

skosito added 2 commits August 8, 2024 16:17

add zero check for whitelist methods

7afddd9

generate

ec450b2

skosito requested a review from fbac August 8, 2024 14:21

fbac approved these changes Aug 8, 2024

View reviewed changes

Base automatically changed from improve-v2-coverage to main August 9, 2024 12:42

skosito added 3 commits August 9, 2024 15:43

Merge branch 'main' into whitelist-custody

99fe0cd

fixes after merge

c18cc95

generate

36e12da

skosito merged commit f975656 into main Aug 9, 2024
9 checks passed

skosito deleted the whitelist-custody branch August 9, 2024 13:49

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: add whitelisting to erc20 custody v2 #296

feat: add whitelisting to erc20 custody v2 #296

skosito commented Aug 7, 2024 •

edited by coderabbitai bot

Loading

coderabbitai bot commented Aug 7, 2024 •

edited

Loading

Review failed

Chat

CodeRabbit Commands (invoked as PR comments)

CodeRabbit Configuration File (`.coderabbit.yaml`)

Documentation and Community

codecov-commenter commented Aug 7, 2024 •

edited

Loading

fbac Aug 8, 2024

skosito Aug 8, 2024 •

edited

Loading

fbac Aug 8, 2024 •

edited

Loading

lumtis left a comment

feat: add whitelisting to erc20 custody v2 #296

feat: add whitelisting to erc20 custody v2 #296

Conversation

skosito commented Aug 7, 2024 • edited by coderabbitai bot Loading

Summary by CodeRabbit

coderabbitai bot commented Aug 7, 2024 • edited Loading

Review failed

Walkthrough

Changes

Sequence Diagram(s)

Chat

CodeRabbit Commands (invoked as PR comments)

CodeRabbit Configuration File (.coderabbit.yaml)

Documentation and Community

codecov-commenter commented Aug 7, 2024 • edited Loading

Codecov Report

fbac Aug 8, 2024

Choose a reason for hiding this comment

skosito Aug 8, 2024 • edited Loading

Choose a reason for hiding this comment

fbac Aug 8, 2024 • edited Loading

Choose a reason for hiding this comment

lumtis left a comment

Choose a reason for hiding this comment

skosito commented Aug 7, 2024 •

edited by coderabbitai bot

Loading

coderabbitai bot commented Aug 7, 2024 •

edited

Loading

CodeRabbit Configuration File (`.coderabbit.yaml`)

codecov-commenter commented Aug 7, 2024 •

edited

Loading

skosito Aug 8, 2024 •

edited

Loading

fbac Aug 8, 2024 •

edited

Loading