ExocoreNetwork / exocore-contracts

5 stars 5 forks source link

feat: tvl limits on the client chain #93

Closed MaxMustermann2 closed 2 months ago

MaxMustermann2 commented 2 months ago

feat(tvl-limit): impose tvl limit on client chain

The TVL limit is imposed on the client chain, which also imposes the fact that nobody can deposit more than the total supply of the tokens.

Summary by CodeRabbit

Summary by CodeRabbit

coderabbitai[bot] commented 2 months ago

Walkthrough

The changes involve significant modifications across multiple contracts, focusing on stricter compilation rules, token supply management, nonce tracking, and enhanced error handling. The foundry.toml file now mandates that compilation fails if warnings are present. The DeployExocoreGatewayOnly contract has been updated to include the exocoreProxyAdmin address in its serialization process. The SetupScript contract has shifted to directly referencing token supplies instead of TVL limits, and new mappings for nonces have been introduced in the ExocoreDeployer contract. Additionally, error handling has been improved across various contracts.

Changes

Files Change Summary
foundry.toml Added deny_warnings = true to enforce stricter compilation rules, ensuring compilation fails if warnings are present.
script/10_DeployExocoreGatewayOnly.s.sol Updated serialization to include exocoreProxyAdmin address in the DeployExocoreGatewayOnly contract.
script/3_Setup.s.sol Modified token supply handling by replacing TVL limits with total supply metrics and adjusting the logic for populating the tvlLimits array.
test/foundry/ExocoreDeployer.t.sol Introduced new mappings for outboundNonces and inboundNonces, and updated logic for managing whitelisted tokens and TVL limits based on token supply.
script/deployedExocoreGatewayOnly.json Added exocoreProxyAdmin field to the JSON configuration with the corresponding address.
src/core/Bootstrap.sol Enhanced token whitelisting by adding TVL limits as parameters in the initialize and addWhitelistTokens functions, along with new error checks for array lengths.
src/core/ClientGatewayLzReceiver.sol Updated payload decoding to include tvlLimit and adjusted logic for managing token limits in requests.
src/core/ExocoreGateway.sol Reordered parameters in addWhitelistToken and removed tvlLimit from updateWhitelistToken, allowing zero values for inactive tokens.
src/interfaces/IExocoreGateway.sol Updated addWhitelistToken and updateWhitelistToken function signatures to reflect changes in parameter handling.
src/libraries/Errors.sol Introduced new error types for improved error handling related to token management and removed outdated error types.
src/storage/BootstrapStorage.sol Added tvlLimit parameter to _deployVault function for vault initialization and a new function to count whitelisted tokens.
src/storage/ClientChainGatewayStorage.sol Introduced a constant for the length of add whitelist token requests and removed the previous constant for token address length.
src/storage/ExocoreGatewayStorage.sol Added constants defining lengths for associate and dissociate operator requests.
src/storage/GatewayStorage.sol Introduced MessageExecuted event and InvalidRequestLength error for better message processing and error handling.
test/foundry/unit/ClientChainGateway.t.sol Modified message construction for adding tokens to whitelist, incorporating dynamic values based on token state.
test/foundry/unit/ExocoreGateway.t.sol Updated function signatures and error messages to align with new token management logic, reflecting changes in handling total supply.
test/mocks/AssetsMock.sol Removed tvlLimit from registerToken and updateToken functions for streamlined usage.
test/mocks/ExocoreGatewayMock.sol Adjusted constructor and updateWhitelistToken function to allow zero tvlLimit for inactive tokens, reflecting a shift in validation logic.

Possibly related PRs

🐰 In a garden lush and bright,
I hop and dance with pure delight.
New limits set for tokens fair,
A world of numbers, fresh and rare.
With every hop, I celebrate,
For changes made, oh, how great! 🥕


Recent review details **Configuration used: CodeRabbit UI** **Review profile: CHILL**
Commits Files that changed from the base of the PR and between 83e96a5e1bea2f334207b68be3f6ef58780ab7af and a3b5c7ca1b4b5ff1248f6057187ee2ccae9fe334.
Files selected for processing (15) * script/3_Setup.s.sol (2 hunks) * src/core/Bootstrap.sol (7 hunks) * src/core/ClientGatewayLzReceiver.sol (3 hunks) * src/core/ExocoreGateway.sol (3 hunks) * src/interfaces/IExocoreGateway.sol (1 hunks) * src/libraries/Errors.sol (3 hunks) * src/storage/BootstrapStorage.sol (3 hunks) * src/storage/ClientChainGatewayStorage.sol (1 hunks) * src/storage/ExocoreGatewayStorage.sol (2 hunks) * src/storage/GatewayStorage.sol (3 hunks) * test/foundry/ExocoreDeployer.t.sol (7 hunks) * test/foundry/unit/ClientChainGateway.t.sol (1 hunks) * test/foundry/unit/ExocoreGateway.t.sol (6 hunks) * test/mocks/AssetsMock.sol (1 hunks) * test/mocks/ExocoreGatewayMock.sol (4 hunks)
Files skipped from review due to trivial changes (3) * src/storage/ExocoreGatewayStorage.sol * test/foundry/ExocoreDeployer.t.sol * test/mocks/AssetsMock.sol
Files skipped from review as they are similar to previous changes (5) * src/core/Bootstrap.sol * src/core/ClientGatewayLzReceiver.sol * src/libraries/Errors.sol * src/storage/ClientChainGatewayStorage.sol * test/foundry/unit/ClientChainGateway.t.sol
Additional comments not posted (17)
src/interfaces/IExocoreGateway.sol (2)
`51-51`: **LGTM, but update the documentation and the client code.** The changes to the `addWhitelistToken` function are approved: - Moving the `tvlLimit` parameter to the end of the parameter list improves the readability and maintainability of the code. - Updating the `tvlLimit` parameter description improves the clarity of the code. - Changing the `tvlLimit` parameter type from `uint256` to `uint128` is justified by the comment that explains that it can fit 1 trillion tokens with 18 decimals, which is sufficient for most use cases. It also improves the compatibility with Solana. However, please make sure to update the documentation and the client code to reflect these changes, as they are breaking changes. Also applies to: 54-55, 62-63 --- `68-68`: **LGTM, but update the documentation and the client code.** The changes to the `updateWhitelistToken` function are approved: - Removing the `tvlLimit` parameter simplifies the function's signature and emphasizes the necessity of the token's existence in the whitelist before any updates can be made. - The function's documentation has been updated to reflect these changes, enhancing clarity regarding the roles of the parameters. However, please make sure to update the documentation and the client code to reflect these changes, as they are breaking changes. Also applies to: 71-71
src/storage/GatewayStorage.sol (3)
`43-46`: **LGTM!** The newly added `MessageExecuted` event provides useful visibility into message processing and execution. The event parameters `act` and `nonce` are appropriate for tracking and debugging purposes. --- `61-65`: **LGTM!** The newly added `InvalidRequestLength` error enhances error management by providing the expected and actual lengths of requests. This helps in identifying and fixing issues related to invalid request lengths. --- `106-114`: **LGTM!** The newly added `_validatePayloadLength` function is a useful addition that allows the contract to enforce stricter checks on incoming data. It correctly validates the payload length against the expected length and reverts with the `InvalidRequestLength` error if there is a mismatch. This helps in ensuring data integrity and preventing unexpected behavior.
script/3_Setup.s.sol (3)
`100-100`: **LGTM!** The type change from `uint256` to `uint128` for the `tvlLimits` array is approved. It reduces storage size while still allowing sufficiently large values for token value limits. --- `108-108`: **LGTM!** Setting the first element of `tvlLimits` to `restakeToken.totalSupply() / 5` is approved. This aligns with the phased approach and ensures the limit stays proportional to the token's total supply. --- `116-116`: **LGTM!** Setting the second element of `tvlLimits` to 0 for native restaking is approved. This effectively removes any limit on native restaking, aligning with the intended behavior.
test/mocks/ExocoreGatewayMock.sol (2)
`175-187`: **LGTM!** The changes to the constructor function are approved: - The change in the parameter order for `tvlLimit` does not affect the functionality. - Allowing a zero value for `tvlLimit` for inactive tokens is a valid change. - The comment regarding the allowance of a TVL limit of zero for inactive tokens provides clarity. --- `207-216`: **LGTM!** The changes to the `updateWhitelistToken` function are approved: - The removal of the `tvlLimit` parameter is consistent with the shift from a TVL-centric approach to one that prioritizes total supply. - The requirement that `metaData` must not be empty reinforces the new logic.
src/storage/BootstrapStorage.sol (2)
Line range hint `355-373`: **LGTM!** The changes to the `_deployVault` function are approved. The addition of the `tvlLimit` parameter and its usage in vault initialization enhances the functionality by allowing a TVL limit to be set for each vault during deployment. --- `378-380`: **LGTM!** The addition of the `_getWhitelistedTokensCount` function is approved. Although the function seems redundant, as mentioned in the comments, it provides a shared implementation for getting the count of whitelisted tokens, which can be used by both the Bootstrap and ClientChainGateway contracts.
src/core/ExocoreGateway.sol (2)
`207-207`: **Breaking change: TVL limit can no longer be updated. Update all callers.** The `tvlLimit` parameter has been removed from the function signature and the internal logic, which is a breaking change. Ensure all callers of this function are updated to match the new signature. The TVL limit can no longer be updated through this function. It must be updated separately on the client chain. To verify that all callers have been updated, run: Also applies to: 215-216
Verification successful --- **Verification successful: Callers of `updateWhitelistToken` have been updated.** The `tvlLimit` parameter has been removed from the function signature, and all callers in the codebase have been updated to match this change. The function is now called with the updated signature, confirming that the breaking change has been addressed.
Scripts executed The following scripts were executed for the analysis: Script: ```shell #!/bin/bash # Description: Verify all callers of `updateWhitelistToken` have been updated to match the new function signature. # Test: Search for calls to the function. Expect: Only occurrences of the new signature without `tvlLimit`. rg --type solidity -A 5 $'ExocoreGateway\(.*\)\.updateWhitelistToken\(' ``` Length of output: 105 --- Script: ```shell #!/bin/bash # Find all Solidity files and search for calls to `updateWhitelistToken`. fd --extension sol --exec rg 'updateWhitelistToken\(' ``` Length of output: 872
--- `177-178`: **Breaking change: Update all callers. Consider including `tvlLimit` in `registerToken`.** The function signature has been updated to move the `tvlLimit` parameter to the end, which is a breaking change. Ensure all callers of this function are updated to match the new signature. Setting a `tvlLimit` of zero is now permitted to add an inactive token that can be activated later by updating the TVL limit on the client chain. Consider including the `tvlLimit` in the call to `ASSETS_CONTRACT.registerToken` for consistency, instead of passing it separately in the interchain message. To verify that all callers have been updated, run: Also applies to: 185-186, 199-199
test/foundry/unit/ExocoreGateway.t.sol (3)
`529-529`: **LGTM!** The update to the `MESSAGE_LENGTH` constant to include an additional `uint128` parameter for the total supply limit is correct and aligns with the changes made to the `addWhitelistToken` function. --- `545-545`: **LGTM!** The addition of the `uint256` parameter initialized to `0` in the `addWhitelistToken` function calls across various test cases is correct. This parameter likely represents the total supply limit for the token. Also applies to: 554-554, 562-562, 570-570, 578-578, 586-587 --- `613-613`: **LGTM!** The changes in the `UpdateWhitelistTokens` contract, including the update to the `MESSAGE_LENGTH` constant, the addition of the `uint256` parameter in the `addWhitelistToken` function calls, and the removal of the `tvlLimit` field from the `TokenDetails` struct, are consistent with the modifications made in the `AddWhitelistTokens` contract and align with the terminology change to use total supply instead. Also applies to: 618-618, 626-627, 642-642, 649-649, 655-655, 661-661, 668-668
--- 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 - [X](https://twitter.com/intent/tweet?text=I%20just%20used%20%40coderabbitai%20for%20my%20code%20review%2C%20and%20it%27s%20fantastic%21%20It%27s%20free%20for%20OSS%20and%20offers%20a%20free%20trial%20for%20the%20proprietary%20code.%20Check%20it%20out%3A&url=https%3A//coderabbit.ai) - [Mastodon](https://mastodon.social/share?text=I%20just%20used%20%40coderabbitai%20for%20my%20code%20review%2C%20and%20it%27s%20fantastic%21%20It%27s%20free%20for%20OSS%20and%20offers%20a%20free%20trial%20for%20the%20proprietary%20code.%20Check%20it%20out%3A%20https%3A%2F%2Fcoderabbit.ai) - [Reddit](https://www.reddit.com/submit?title=Great%20tool%20for%20code%20review%20-%20CodeRabbit&text=I%20just%20used%20CodeRabbit%20for%20my%20code%20review%2C%20and%20it%27s%20fantastic%21%20It%27s%20free%20for%20OSS%20and%20offers%20a%20free%20trial%20for%20proprietary%20code.%20Check%20it%20out%3A%20https%3A//coderabbit.ai) - [LinkedIn](https://www.linkedin.com/sharing/share-offsite/?url=https%3A%2F%2Fcoderabbit.ai&mini=true&title=Great%20tool%20for%20code%20review%20-%20CodeRabbit&summary=I%20just%20used%20CodeRabbit%20for%20my%20code%20review%2C%20and%20it%27s%20fantastic%21%20It%27s%20free%20for%20OSS%20and%20offers%20a%20free%20trial%20for%20proprietary%20code)
Tips ### Chat There are 3 ways to chat with [CodeRabbit](https://coderabbit.ai): - Review comments: Directly reply to a review comment made by CodeRabbit. Example: - `I pushed a fix in commit .` - `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 using 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. ### Other keywords and placeholders - Add `@coderabbitai ignore` anywhere in the PR description to prevent this PR from being reviewed. - Add `@coderabbitai summary` to generate the high-level summary at a specific location in the PR description. - Add `@coderabbitai` or `@coderabbitai title` anywhere in the PR title to generate the title automatically. ### 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](https://docs.coderabbit.ai/guides/configure-coderabbit) 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](https://coderabbit.ai/docs) for detailed information on how to use CodeRabbit. - Join our [Discord Community](https://discord.com/invite/GsXnASn26c) to get help, request features, and share feedback. - Follow us on [X/Twitter](https://twitter.com/coderabbitai) for updates and announcements.
MaxMustermann2 commented 2 months ago

LGTM, the only concern is that we can only force that tvlLimist <= totalSupply while registering, but later both variables could be updated independently and we can not put constraints on them anymore. In case at some point, owing to misconfiguration, the unconsumed tvlLimit is greater than the unconsumed totalSupply, there is a possibility that deposit could fail. Or could we maintain only the tvlLimit on client chain to avoid such risks? do you think the benefits of maintaining the two limits win the risks and burdens?

I reassessed the situation after reading your message and now I believe the probability of misconfiguration isn't negligible. I have implemented the following checks.

Care has been taken to ensure that in-flight LZ messages do not cause trouble for this approach. For example, if the TVL limit is 400m and the total supply is 600m, increasing the TVL limit to 450m should be approved. However, it may be possible that there is an in-flight LZ message pending for validation by the client chain, containing a proposed decrease of the total supply to 420m. If there is (at least) one, the TVL limit change is rejected by Exocore.

With the above changes, the risk of deposit failures is extremely low since the TVL limit is always less than or equal to the total supply as on Exocore. This applies even during the Bootstrap phase and its offset duration: as long as TVL limits don't exceed the total supply when the contract is active, any changes to total supply during the offset duration will not result in the inconsistency that the TVL limit exceeds the total supply stored in the genesis file.

Remember that the objective is always to prevent the failure of deposits on Exocore; they may still fail on the client chain. Hence, the TVL limit can be higher than the total supply on the client chain. In that event, the transferFrom will itself fail.

I separately considered moving to a system wherein there's only one deposit limit (the TVL limit), however, I felt that it's better to have multiple layers of defense for permissionless DeFi products.

I paste the commit message below for ease of reference.

commit 25eee39955f3ea80a5ff76b3f3be2a793debff8b Author: MaxMustermann2 82761650+MaxMustermann2@users.noreply.github.com Date: Sat Sep 7 14:19:08 2024 +0000

feat: prevent misconfig of supply and tvl limits

The objective of this change is to prevent deposit failures by ensuring
that the tvlLimit (as enforced on the client chain) <= totalSupply (as
enforced by Exocore). Note that, with time, the totalSupply stored on
Exocore may become outdated as well; however, it is a secondary check
which occurs only after tokens are transferred into the Vault on the
client chain.

Since the addition of tokens to the whitelist happens through Exocore,
it can be enforced easily that the tvlLimit of said token is less than
the total supply (as known to Exocore).

Whenever a transaction to update the tvlLimit on a client chain is
received, it is handled on a case-by-case basis.
- A decrease in tvl limit is always allowed.
- An increase in tvl limit is forwarded to Exocore for validation.

Exocore receives this validation request, and responds in the
affirmative if the new tvl limit <= supply of the token (again,
according to Exocore). It responds in the negative, otherwise.

Reciprocally, whenever an attempt is made to change the recorded value
of total supply corresponding to a token on Exocore, here's what
happens.
- An increase in the total supply is always allowed.
- A decrease in the total supply is forwarded to the client chain for
  validation.

when the client chain receives this validation request, it responds in
the affirmative if the tvl limit <= new total supply (which may be
totally different from the actual total supply due to changes in the
number when the message was in-flight). Otherwise, it responds in the
negative.

One caveat for both of these validations is that if a validation request
arising from the validating chain is already in-flight, the validation
is rejected. This is to ensure that in-flight messages cannot result in
a misconfiguration.

For example, if the TVL limit is increased from 400m to 500m and the
total supply is 600m, it will be approved. However, if, at the same
time, there is a validation request sent to the client chain by Exocore
to verify if the total supply can be reduced to 450m, it could cause
inconsistency. In this event, the TVL limit increase is rejected and the
total supply decrease will be approved.
MaxMustermann2 commented 2 months ago

To be merged with https://github.com/ExocoreNetwork/exocore/pull/181