feat: deploy fee contract for transaction processing #807
Conversation
WalkthroughThe pull request introduces significant updates across multiple files, primarily focusing on the integration of bridge fee management within the application. Key changes include the addition of new keepers for handling bridge fees, modifications to the initialization logic to ensure delegations are present before deploying contracts, and enhancements to error handling. New contracts and interfaces are defined to facilitate interactions with Ethereum smart contracts, while existing contracts are updated to utilize role-based access control for improved security. Additionally, several redundant files related to previous contract bindings have been removed. Changes
Possibly related PRs
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? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
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)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 10
🧹 Outside diff range and nitpick comments (13)
contract/interface.go (2)
19-21: Add documentation for AccountKeeper interface.Please add GoDoc comments describing:
- The interface's purpose
- The
GetModuleAddressmethod's parameter and return value- Error handling behavior for non-existent module names
1-21: Well-designed interface separation.The separation of
EvmKeeperandAccountKeeperinterfaces follows good design principles:
- Clear separation of concerns
- Focused responsibilities
- Enables easy mocking for tests
- Provides flexibility for different implementations
solidity/contracts/bridge/BridgeProxy.sol (2)
1-5: Consider pinning to a specific Solidity version.Using
^0.8.0allows any 0.8.x version, which could lead to inconsistencies. Consider pinning to a specific version (e.g.,0.8.19) to ensure consistent behavior across deployments.-pragma solidity ^0.8.0; +pragma solidity 0.8.19;
1-29: Consider additional security measures for this critical infrastructure.As this proxy contract is a crucial part of the bridge infrastructure:
- Add extensive test coverage, including upgrade scenarios
- Document clear upgrade procedures and emergency response plans
- Consider implementing a timelock for upgrades
- Add events for important state changes (initialization, upgrades)
- Consider having the contract audited before deployment
Would you like assistance in implementing any of these recommendations?
contract/compile.sh (1)
34-35: LGTM! Consider grouping related contracts together.The contract additions align well with the PR objectives for deploying fee contracts. The separation of interfaces and implementations is good practice.
Consider grouping related contracts together for better maintainability:
contracts=(WFXUpgradable FIP20Upgradable IStaking IError ERC1967Proxy) -contracts+=(IFxBridgeLogic IBridgeCallback ICrosschain) -contracts+=(BridgeFeeQuote BridgeFeeOracle BridgeProxy) +# Bridge and Fee related contracts +contracts+=(IFxBridgeLogic IBridgeCallback ICrosschain BridgeFeeQuote BridgeFeeOracle BridgeProxy) contracts+=(CrosschainTest StakingTest ERC721TokenTest)contract/bridge_fee_quote.go (1)
21-28: Add contract address validation in constructorThe constructor accepts a contract address as a string but doesn't validate if it's a valid Ethereum address. Consider adding validation to prevent runtime errors.
func NewBridgeFeeQuoteKeeper(caller Caller, contract string) BridgeFeeQuoteKeeper { + if !common.IsHexAddress(contract) { + panic("invalid contract address") + } return BridgeFeeQuoteKeeper{ Caller: caller, abi: GetBridgeFeeQuote().ABI, from: common.BytesToAddress(authtypes.NewModuleAddress(types.ModuleName).Bytes()), contract: common.HexToAddress(contract), } }client/client_test.go (1)
192-192: Update the module accounts comment to reflect new contracts.The comment listing module accounts appears to be outdated. With the addition of bridge fee contracts, please update the comment to include all module accounts for better maintainability.
contract/contract.go (1)
51-71: Well-structured contract management architecture.The use of proxy pattern for bridge fee contracts provides upgrade flexibility. The clear separation between proxy and implementation contracts, along with consistent getter functions, makes the system maintainable and extensible.
Also applies to: 106-116
contract/bridge_fee_oracle.go (2)
33-55: Refactor Role Retrieval Methods to Reduce Code DuplicationThe methods
GetOwnerRole,GetUpgradeRole, andGetQuoteRolehave similar implementations. To enhance maintainability and reduce code duplication, consider refactoring them into a single method that accepts the role name as a parameter.Here's how you could refactor the code:
func (k BridgeFeeOracleKeeper) GetRole(ctx context.Context, roleName string) (common.Hash, error) { var res struct{ Role common.Hash } if err := k.QueryContract(sdk.UnwrapSDKContext(ctx), k.from, k.contract, k.abi, roleName, &res); err != nil { return common.Hash{}, err } return res.Role, nil }Then, update the specific role retrieval methods to use
GetRole:func (k BridgeFeeOracleKeeper) GetOwnerRole(ctx context.Context) (common.Hash, error) { return k.GetRole(ctx, "OWNER_ROLE") } func (k BridgeFeeOracleKeeper) GetUpgradeRole(ctx context.Context) (common.Hash, error) { return k.GetRole(ctx, "UPGRADE_ROLE") } func (k BridgeFeeOracleKeeper) GetQuoteRole(ctx context.Context) (common.Hash, error) { return k.GetRole(ctx, "QUOTE_ROLE") }
57-62: Consider Error Handling ConsistencyIn the methods
GrantRoleandSetDefaultOracle, errors returned byApplyContractare propagated directly. Ensure that error handling is consistent across all methods, and consider providing additional context or logging where applicable to aid in debugging.contract/bridge_fee.go (2)
66-99: Refactor role initialization to remove duplicated codeThe functions
initBridgeFeeOracleandinitBridgeFeeQuotecontain similar code blocks for retrieving and granting roles. Consider refactoring the common logic into a shared helper function to reduce code duplication and enhance maintainability.Also applies to: 101-130
53-63: Enhance error messages with contextual informationCurrently, errors are returned directly without additional context. To improve debugging and traceability, consider wrapping the errors with additional messages indicating the operation that failed.
For example:
- return err + return fmt.Errorf("failed to grant role: %w", err)Ensure to import the
fmtpackage if it's not already included.Also applies to: 71-97, 107-129
app/upgrades/v8/upgrade.go (1)
108-108: Address the TODO: Set Bridge Fee Contract Owner AddressThere is a TODO comment indicating that the bridge fee contract owner address needs to be set before the mainnet upgrade. Please ensure to set the appropriate owner address to avoid potential issues during deployment.
Would you like assistance in generating the code to set the bridge fee contract owner address?
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (16)
app/app.go(4 hunks)app/upgrades/v8/upgrade.go(3 hunks)client/client_test.go(2 hunks)contract/bridge_fee.go(1 hunks)contract/bridge_fee_oracle.go(1 hunks)contract/bridge_fee_oracle.sol.go(1 hunks)contract/bridge_fee_quote.go(1 hunks)contract/bridge_proxy.sol.go(1 hunks)contract/compile.sh(1 hunks)contract/contract.go(3 hunks)contract/ibridge_fee_quote.sol.go(0 hunks)contract/interface.go(1 hunks)contract/types.go(1 hunks)solidity/contracts/bridge/BridgeFeeOracle.sol(5 hunks)solidity/contracts/bridge/BridgeFeeQuote.sol(6 hunks)solidity/contracts/bridge/BridgeProxy.sol(1 hunks)
💤 Files with no reviewable changes (1)
- contract/ibridge_fee_quote.sol.go
✅ Files skipped from review due to trivial changes (3)
- contract/bridge_fee_oracle.sol.go
- contract/bridge_proxy.sol.go
- contract/types.go
🔇 Additional comments (23)
contract/interface.go (2)
1-11: LGTM! Package and imports are well-organized.
The imports cover all necessary dependencies for EVM contract interactions and Cosmos SDK integration.
13-17: Add interface and method documentation.
The EvmKeeper interface handles critical contract operations. Please add GoDoc comments describing:
- The interface's purpose and responsibilities
- Each method's parameters, return values, and potential error cases
- Any validation requirements for input parameters
Consider standardizing context usage.
The interface uses different context types:
DeployContractandCreateContractWithCodeusesdk.ContextApplyContractusescontext.Context
This inconsistency might lead to confusion. Consider standardizing to sdk.Context if possible.
Let's check if this context type mixing is a common pattern:
solidity/contracts/bridge/BridgeProxy.sol (3)
7-7: LGTM! Good use of custom errors.
Using a custom error instead of a revert string is gas-efficient and provides clear error handling.
9-9: LGTM! Proper inheritance structure.
The contract correctly inherits from both Proxy and ERC1967Upgrade, following the proxy upgrade pattern.
17-28: LGTM! Clean implementation of _implementation.
The function correctly overrides the Proxy's virtual function and is well-documented.
contract/compile.sh (1)
34-35: Verify removal of old fee quote interfaces.
Let's ensure there are no lingering references to the removed interfaces (IFxBridgeFeeQuote, IBridgeFeeQuote) in the codebase.
contract/bridge_fee_quote.go (2)
31-45: LGTM! Query methods are well-implemented
The query methods follow best practices with proper error handling and context management.
67-69: Verify role management security
The GrantRole method should be called only by accounts with appropriate permissions. Consider adding role checks or documenting the assumption that role checks are handled by the contract.
✅ Verification successful
The previous ast-grep pattern didn't yield results, likely because the contract code might be in a different format or location. Let's try searching with ripgrep to find role-related implementations and access control patterns.
Based on the search results, I can see that both contracts (BridgeFeeQuote and BridgeFeeOracle) implement role-based access control with functions like hasRole, grantRole, and events like RoleGranted. The contracts appear to be using OpenZeppelin's AccessControl pattern, which provides built-in role checks.
Role checks are properly implemented in the contract
The contract implements OpenZeppelin's AccessControl pattern which ensures that only accounts with appropriate roles can call privileged functions. The grantRole function inherits these checks from the implementation. This is evidenced by:
- The presence of role-related events (
RoleGranted,RoleRevoked,RoleAdminChanged) - Standard AccessControl functions (
hasRole,grantRole,revokeRole) - Role constants defined in the contract (
DEFAULT_ADMIN_ROLE,OWNER_ROLE)
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Check if the contract implements role checks
ast-grep --pattern 'function grantRole(bytes32 role, address account) $$$'
Length of output: 76
Script:
#!/bin/bash
# Search for role-related code in the contract directory
rg -A 5 "grantRole|hasRole|AccessControl" contract/
# Search for any role-related imports or inheritance
rg -A 2 "@openzeppelin/contracts/access" contract/
Length of output: 76361
client/client_test.go (1)
192-192: Verify sequence number adjustments.
The sequence number assertions have been updated:
- toAddress: from 15+i to 19+i
- ethAddress: from 18 to 22
These changes appear to reflect additional transactions during initialization, likely due to the new bridge fee contract deployment. However, let's verify this assumption.
Also applies to: 227-227
✅ Verification successful
Let me gather more information about the sequence number changes by examining the test setup and initialization.
Let me try a different approach to examine the test file and sequence number changes.
After analyzing the test file, I can now provide a final response:
The sequence number changes are correct and verified.
The sequence number changes in the test assertions are justified by the initialization process:
- The test setup shows that accounts are initialized in a specific order, with module accounts being created first
- Comments in the code explicitly list the accounts: "acconts is 0.initAccount, 1.fee_collector, 2.distribution, 3.bonded_tokens_pool, 4.not_bonded_tokens_pool, 5.gov, 6.mint, 7.crosschain, 8.evm, 9.0x..1001, 10.0x..1002, 11.erc20, 12.wfx-contract"
- The sequence numbers (19+i for toAddress and 22 for ethAddress) align with this initialization sequence and the bridge fee contract deployment
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify the initialization flow and contract deployments that affect sequence numbers
# Test 1: Check for bridge fee contract initialization in app
rg -A 5 "InitChainer.*bridge.*fee"
# Test 2: Look for contract deployment transactions
rg -A 5 "DeployBridgeFee.*Contract"
Length of output: 1880
Script:
#!/bin/bash
# Check the test setup and initialization in client_test.go
rg -B 5 -A 10 "TestQueryAccount" client/client_test.go
# Look for any other sequence number assertions or setup
rg "NewBaseAccount.*sequence" client/client_test.go
# Check for any bridge fee related setup in test
rg "bridge.*fee.*setup" client/client_test.go
Length of output: 156
Script:
#!/bin/bash
# Get the full content of client_test.go to understand the test context
cat client/client_test.go
# Look for any setup functions that might affect sequence numbers
rg -A 10 "func \(.*Suite\) Setup" client/client_test.go
# Check for any initialization or deployment transactions
rg -A 5 "Deploy.*Contract.*test" --type go
Length of output: 20404
contract/contract.go (4)
20-23: LGTM: Contract addresses follow consistent pattern.
The new contract addresses maintain the sequential pattern established by existing contracts.
33-34: LGTM: Clear source documentation for deployment code.
The comments clearly indicate the source Solidity files for the deployment code, improving traceability.
Also applies to: 40-41
106-116: LGTM: Getter functions follow consistent pattern.
The new getter functions maintain consistency with existing ones in terms of naming, visibility, and return types.
51-71: Verify empty Code fields for bridge fee contracts.
While bridgeProxy has deployment code, both bridgeFeeQuote and bridgeFeeOracle have empty Code fields. If this is intentional (e.g., using proxy pattern), please confirm. Otherwise, ensure the deployment code is added.
✅ Verification successful
Let me gather more information about the proxy pattern implementation.
Let me gather more information about the proxy pattern implementation.
Empty Code fields are correct - contracts use proxy pattern
The empty Code fields for bridgeFeeQuote and bridgeFeeOracle are intentional as these contracts use the proxy pattern. Evidence:
-
Both contracts are deployed through
deployBridgeProxy()which:- Deploys the logic contract using the provided ABI and Bin
- Uses proxy pattern to delegate calls to the logic implementation
-
The deployment and initialization flow:
- First deploys proxy contracts to predefined addresses (0x1005 and 0x1006)
- Then initializes them through
initBridgeFeeQuote()andinitBridgeFeeOracle() - The empty Code fields in the Contract structs are correct since the actual code lives at the logic contract addresses
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check if the contracts use proxy pattern
# Test: Search for proxy setup in bridge fee contracts
rg -A 5 "bridgeFeeQuote|bridgeFeeOracle" --type go
Length of output: 5918
Script:
#!/bin/bash
# Check for proxy pattern implementation and deployment code
# Search for contract deployment and initialization
rg -A 5 "Initialize|deployBridgeProxy" --type go
# Search for proxy-related code
ast-grep --pattern 'func deployBridgeProxy($$$) { $$$ }'
Length of output: 256049
Script:
#!/bin/bash
# Check if bridgeFeeQuote and bridgeFeeOracle are deployed through proxy pattern
# Search for proxy deployment and initialization
rg -A 10 "deployBridgeProxy|initBridgeFeeQuote|initBridgeFeeOracle" --type go
# Search for proxy addresses
rg "BridgeFeeAddress|BridgeFeeOracleAddress" --type go
Length of output: 5181
solidity/contracts/bridge/BridgeFeeOracle.sol (4)
22-23: Role constants OWNER_ROLE and UPGRADE_ROLE are properly defined
The addition of OWNER_ROLE and UPGRADE_ROLE constants follows best practices for defining roles in access control.
75-75: Appropriate use of OWNER_ROLE in blackOracle function
The blackOracle function now requires OWNER_ROLE for access control. This change increases security by restricting access to authorized users.
86-86: Appropriate use of OWNER_ROLE in setDefaultOracle function
Requiring OWNER_ROLE for the setDefaultOracle function enhances security by limiting this action to specific roles.
100-100: Restricting upgrade authorization to UPGRADE_ROLE
The _authorizeUpgrade function now requires UPGRADE_ROLE, ensuring that only authorized roles can perform contract upgrades, enhancing security.
app/upgrades/v8/upgrade.go (1)
98-98: Potential Issue with Oracles Length Check
The method Len() is used on oracles. Ensure that oracles has a Len() method. If oracles is a slice, you should use len(oracles) == 0 instead.
Run the following script to verify the type of oracles and confirm whether it has a Len() method:
✅ Verification successful
Let me gather more information to understand how oracles is being used in the specific file under review.
Based on the gathered information, I can now provide the final response:
Len() Method Usage is Correct
The code is correct as is. From the previous results, we can see that:
GetAllOraclesreturns atypes.OraclestypeOraclestype is defined inx/crosschain/types/types.goand has a properLen()method implementation
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check if 'oracles' is a slice or has a 'Len()' method.
# Expected Result: Determine whether to use 'len(oracles)' or 'oracles.Len()' in the length check.
# Find the definition of 'GetAllOracles' method
rg 'func \(.*\) GetAllOracles' -A 5
# Search for the type of 'oracles'
rg 'type OracleSet' -A 5
Length of output: 2012
Script:
#!/bin/bash
# Check the context around the error return in the upgrade file
rg -B 10 "return fromVM, errors.new\(\"no oracle found\"\)" app/upgrades/v8/upgrade.go
# Check for any Oracles type definition
ast-grep --pattern 'type Oracles'
# Look for the GetAllOracles usage in the upgrade file
rg "GetAllOracles" app/upgrades/v8/upgrade.go -B 5 -A 5
Length of output: 541
solidity/contracts/bridge/BridgeFeeQuote.sol (3)
5-5: Transition to AccessControlUpgradeable for role-based access control
The contract now uses AccessControlUpgradeable instead of OwnableUpgradeable, introducing OWNER_ROLE and UPGRADE_ROLE. This enhances flexibility by allowing multiple addresses to have specific roles.
Ensure that this transition aligns with the overall access control strategy and that all necessary roles are defined and managed correctly.
Also applies to: 28-28, 35-36
414-414: Ensure correct management of OWNER_ROLE in access control
The functions registerChain, registerTokenName, updateOracleContract, and updateMaxQuoteIndex now use onlyRole(OWNER_ROLE) instead of onlyOwner. Verify that all necessary addresses have been granted the OWNER_ROLE, and that there are no unintended access restrictions due to this change.
Run the following script to confirm OWNER_ROLE assignments:
#!/bin/bash
# Description: Verify that `OWNER_ROLE` is granted to the correct addresses.
rg "_grantRole\(OWNER_ROLE"Ensure that the role is granted appropriately to maintain the intended access permissions.
Also applies to: 426-426, 444-444, 451-451
458-460: Verify assignment of UPGRADE_ROLE for upgrade authorization
The _authorizeUpgrade function now uses onlyRole(UPGRADE_ROLE), which means only addresses with UPGRADE_ROLE can perform contract upgrades. Ensure that the UPGRADE_ROLE is assigned to the appropriate administrative addresses.
Run the following script to find where UPGRADE_ROLE is granted:
This will help confirm that UPGRADE_ROLE is properly assigned in the contract.
✅ Verification successful
Let me search for more information about the UPGRADE_ROLE.
Based on the search results, I can now generate the final response since I have found sufficient information about the UPGRADE_ROLE assignment:
UPGRADE_ROLE is properly assigned during contract initialization
The verification shows that UPGRADE_ROLE is correctly granted to the contract deployer (msg.sender) in both BridgeFeeQuote and BridgeFeeOracle contracts during initialization:
- In BridgeFeeQuote.sol:
_grantRole(UPGRADE_ROLE, msg.sender); - In BridgeFeeOracle.sol:
_grantRole(UPGRADE_ROLE, msg.sender);
This ensures that only the deploying address initially has the ability to perform contract upgrades, following secure upgrade control practices.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Find all instances where `UPGRADE_ROLE` is assigned.
rg "_grantRole\(UPGRADE_ROLE"
Length of output: 208
Script:
#!/bin/bash
# Search for UPGRADE_ROLE definition and usage
rg "UPGRADE_ROLE" -A 2
Length of output: 71602
app/app.go (2)
373-373: Verify that passing 'moduleAddress' twice is appropriate
In the call to DeployBridgeFeeContract, the moduleAddress is passed twice as parameters. Please confirm that this is intentional and that both parameters are meant to be the same address.
374-374: Ensure correct delegator address selection
The code uses delegations[0].DelegatorAddress. If multiple delegations are present, using the first delegator may not be appropriate. Please verify that this is the intended behavior.
| function init(address _logic) public { | ||
| if (_getImplementation() != address(0)) { | ||
| revert AlreadyInitialized(); | ||
| } | ||
| _upgradeTo(_logic); | ||
| } |
There was a problem hiding this comment.
Add access control and input validation to init function.
The current implementation has several security concerns:
- No access control on the init function allows anyone to initialize the proxy
- Missing zero-address validation for _logic parameter
- Vulnerable to front-running attacks during deployment
Consider applying these security improvements:
- function init(address _logic) public {
+ function init(address _logic) public onlyOwner {
+ require(_logic != address(0), "BridgeProxy: zero address");
if (_getImplementation() != address(0)) {
revert AlreadyInitialized();
}
_upgradeTo(_logic);
}Also, consider using an initializer modifier from OpenZeppelin's Initializable contract for additional protection.
Committable suggestion skipped: line range outside the PR's diff.
| function initialize(address _crosschain) public initializer { | ||
| crosschainContract = _crosschain; | ||
|
|
||
| _grantRole(DEFAULT_ADMIN_ROLE, msg.sender); | ||
| _grantRole(UPGRADE_ROLE, msg.sender); | ||
| _grantRole(OWNER_ROLE, msg.sender); | ||
|
|
There was a problem hiding this comment.
Call parent initializers before granting roles
The _grantRole functions are called before the parent initializers __AccessControl_init(), __UUPSUpgradeable_init(), and __ReentrancyGuard_init(). It's important to initialize parent contracts before using their functions to prevent unexpected behavior.
Apply the following diff to correct the initialization order:
function initialize(address _crosschain) public initializer {
crosschainContract = _crosschain;
+ __AccessControl_init();
+ __UUPSUpgradeable_init();
+ __ReentrancyGuard_init();
_grantRole(DEFAULT_ADMIN_ROLE, msg.sender);
_grantRole(UPGRADE_ROLE, msg.sender);
_grantRole(OWNER_ROLE, msg.sender);
- __AccessControl_init();
- __UUPSUpgradeable_init();
- __ReentrancyGuard_init();
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| function initialize(address _crosschain) public initializer { | |
| crosschainContract = _crosschain; | |
| _grantRole(DEFAULT_ADMIN_ROLE, msg.sender); | |
| _grantRole(UPGRADE_ROLE, msg.sender); | |
| _grantRole(OWNER_ROLE, msg.sender); | |
| function initialize(address _crosschain) public initializer { | |
| crosschainContract = _crosschain; | |
| __AccessControl_init(); | |
| __UUPSUpgradeable_init(); | |
| __ReentrancyGuard_init(); | |
| _grantRole(DEFAULT_ADMIN_ROLE, msg.sender); | |
| _grantRole(UPGRADE_ROLE, msg.sender); | |
| _grantRole(OWNER_ROLE, msg.sender); |
| _grantRole(DEFAULT_ADMIN_ROLE, msg.sender); | ||
| _grantRole(UPGRADE_ROLE, msg.sender); | ||
| _grantRole(OWNER_ROLE, msg.sender); | ||
|
|
||
| __AccessControl_init(); |
There was a problem hiding this comment.
Initialize AccessControlUpgradeable before assigning roles
The __AccessControl_init() function should be called before granting any roles to ensure that the internal state is properly initialized. Failing to do so might lead to unexpected behavior in access control.
Apply this diff to correct the order:
function initialize(
address _oracleContract,
uint256 _maxQuoteIndex
) public initializer {
oracleContract = _oracleContract;
maxQuoteIndex = _maxQuoteIndex;
+ __AccessControl_init();
_grantRole(DEFAULT_ADMIN_ROLE, msg.sender);
_grantRole(UPGRADE_ROLE, msg.sender);
_grantRole(OWNER_ROLE, msg.sender);
__UUPSUpgradeable_init();
__ReentrancyGuard_init();
}Committable suggestion skipped: line range outside the PR's diff.
Co-authored-by: todd <81545601+todd-woko@users.noreply.github.com>
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Outside diff range and nitpick comments (12)
contract/types.go (2)
3-3: Add documentation for the magic number.The constant's purpose and the significance of the value
3should be documented. This will help other developers understand why this specific limit was chosen and how it affects the bridge fee quote system.+// DefaultMaxQuoteIndex defines the maximum number of quote indices allowed in the bridge fee system. +// This limit ensures efficient processing of bridge fee quotes while maintaining system performance. const DefaultMaxQuoteIndex = 3
5-8: Enhance struct documentation and consider validation.The struct would benefit from documentation explaining its role in bridge fee contract deployment. Additionally, consider adding validation methods to ensure non-empty values.
+// BridgeDenoms represents the denomination tokens supported for a specific blockchain +// in the bridge fee system. It maps chain names to their respective supported denominations. type BridgeDenoms struct { ChainName string Denoms []string } + +// Validate ensures that BridgeDenoms contains valid data +func (bd BridgeDenoms) Validate() error { + if bd.ChainName == "" { + return fmt.Errorf("chain name cannot be empty") + } + if len(bd.Denoms) == 0 { + return fmt.Errorf("denoms list cannot be empty") + } + return nil +}app/upgrades/v8/upgrade.go (1)
83-90: Enhance error handling for GetBridgeTokens.The error from
GetBridgeTokensshould be wrapped with additional context to help with debugging in production.- bridgeTokens, err := app.Erc20Keeper.GetBridgeTokens(cacheCtx, chain) - if err != nil { - return fromVM, err - } + bridgeTokens, err := app.Erc20Keeper.GetBridgeTokens(cacheCtx, chain) + if err != nil { + return fromVM, fmt.Errorf("failed to get bridge tokens for chain %s: %w", chain, err) + }solidity/contracts/bridge/BridgeFeeQuote.sol (4)
35-37: Consider adding role documentationThe role constants are well-defined using keccak256. Consider adding NatSpec documentation to describe the capabilities of each role.
+ /// @notice Role for administrative operations like registering chains and updating contracts bytes32 public constant OWNER_ROLE = keccak256("OWNER_ROLE"); + /// @notice Role for authorizing contract upgrades bytes32 public constant UPGRADE_ROLE = keccak256("UPGRADE_ROLE");
68-74: Optimize initialization orderConsider grouping related initializations together for better readability:
- __AccessControl_init(); - __UUPSUpgradeable_init(); - __ReentrancyGuard_init(); - - _grantRole(DEFAULT_ADMIN_ROLE, msg.sender); - _grantRole(UPGRADE_ROLE, msg.sender); - _grantRole(OWNER_ROLE, msg.sender); + // Initialize security features + __AccessControl_init(); + __ReentrancyGuard_init(); + + // Initialize upgrade capability + __UUPSUpgradeable_init(); + + // Setup initial roles + _grantRole(DEFAULT_ADMIN_ROLE, msg.sender); + _grantRole(UPGRADE_ROLE, msg.sender); + _grantRole(OWNER_ROLE, msg.sender);
414-420: Consider additional input validationsWhile checking for empty chain name is good, consider adding more validations:
) external onlyRole(OWNER_ROLE) returns (bool) { if (assets[_chainName].isActive) { revert ChainNameAlreadyExists(); } if (_chainName.equal("")) { revert ChainNameInvalid(); } + if (_tokenNames.length == 0) { + revert TokenNameInvalid(); + } + for (uint256 i = 0; i < _tokenNames.length; i++) { + if (_tokenNames[i].equal("")) { + revert TokenNameInvalid(); + } + }
461-463: Add implementation noteThe empty implementation with UPGRADE_ROLE check is correct for UUPS pattern. Consider adding a comment explaining why the function is empty:
- function _authorizeUpgrade( - address - ) internal override onlyRole(UPGRADE_ROLE) {} // solhint-disable-line no-empty-blocks + /// @dev Required by the OZ UUPS module. Adds access control to the upgrade mechanism. + /// @param _ New implementation address (unused in the authorization check) + function _authorizeUpgrade( + address _ + ) internal override onlyRole(UPGRADE_ROLE) { + // Authorization check is handled by the onlyRole modifier + }app/app.go (2)
357-363: Improve error message for delegation validationThe error message for missing delegations could be more descriptive to help with troubleshooting.
- return nil, errors.New("no delegations found") + return nil, errors.New("no delegations found: initial delegations are required for bridge fee contract deployment")
364-369: Consider making bridge denoms configurableThe bridge denoms array is hardcoded with only the ETH chain. Consider making this configurable through genesis state or app configuration.
solidity/contracts/bridge/BridgeFeeOracle.sol (1)
86-89: AddnonReentrantmodifier tosetDefaultOraclefunctionThe
setDefaultOraclefunction modifies state variables and interacts with theoraclesset. To enhance security and prevent potential reentrancy attacks, it's advisable to add thenonReentrantmodifier, consistent with theblackOraclefunction.Apply this diff to add the
nonReentrantmodifier:function setDefaultOracle( address _defaultOracle - ) external onlyRole(OWNER_ROLE) { + ) external onlyRole(OWNER_ROLE) nonReentrant { if (!oracles.contains(_defaultOracle)) { oracleStatus[_defaultOracle] = State(false, true); oracles.add(_defaultOracle);contract/bridge_fee.go (2)
11-44: Add documentation comment for the exported functionDeployBridgeFeeContractThe exported function
DeployBridgeFeeContractlacks a documentation comment. As per Go conventions, exported functions should include comments to explain their purpose and usage for better readability and maintainability.Suggested addition:
+// DeployBridgeFeeContract deploys the bridge fee contracts and initializes them. func DeployBridgeFeeContract( ctx sdk.Context, evmKeeper EvmKeeper, bridgeFeeQuoteKeeper BridgeFeeQuoteKeeper, bridgeFeeOracleKeeper BridgeFeeOracleKeeper, bridgeDenoms []BridgeDenoms, evmModuleAddress, owner, defaultOracleAddress common.Address, ) error {
66-99: Refactor role assignment to reduce code duplicationThe functions
initBridgeFeeOracleandinitBridgeFeeQuoteboth perform role assignments with similar logic. Consider refactoring this repeated code into a helper function to enhance code reusability and maintainability.Example refactor:
func grantRoles(ctx sdk.Context, keeper RoleKeeper, owner common.Address, roleGetters ...func(sdk.Context) (string, error)) error { for _, getRole := range roleGetters { role, err := getRole(ctx) if err != nil { return err } if _, err := keeper.GrantRole(ctx, role, owner); err != nil { return err } } return nil }Modify
initBridgeFeeOracle:func initBridgeFeeOracle( ctx sdk.Context, bridgeFeeOracleKeeper BridgeFeeOracleKeeper, owner, defaultOracleAddress common.Address, ) error { if _, err := bridgeFeeOracleKeeper.Initialize(ctx); err != nil { return err } - ownerRole, err := bridgeFeeOracleKeeper.GetOwnerRole(ctx) - if err != nil { - return err - } - if _, err := bridgeFeeOracleKeeper.GrantRole(ctx, ownerRole, owner); err != nil { - return err - } - upgradeRole, err := bridgeFeeOracleKeeper.GetUpgradeRole(ctx) - if err != nil { - return err - } - if _, err := bridgeFeeOracleKeeper.GrantRole(ctx, upgradeRole, owner); err != nil { - return err - } + if err := grantRoles(ctx, bridgeFeeOracleKeeper, owner, bridgeFeeOracleKeeper.GetOwnerRole, bridgeFeeOracleKeeper.GetUpgradeRole); err != nil { + return err + } if _, err := bridgeFeeOracleKeeper.SetDefaultOracle(ctx, defaultOracleAddress); err != nil { return err } return nil }Similarly modify
initBridgeFeeQuote:func initBridgeFeeQuote( ctx sdk.Context, bridgeFeeQuoteKeeper BridgeFeeQuoteKeeper, bridgeDenoms []BridgeDenoms, owner common.Address, ) error { if _, err := bridgeFeeQuoteKeeper.Initialize(ctx, common.HexToAddress(BridgeFeeOracleAddress), big.NewInt(DefaultMaxQuoteIndex)); err != nil { return err } - ownerRole, err := bridgeFeeQuoteKeeper.GetOwnerRole(ctx) - if err != nil { - return err - } - if _, err := bridgeFeeQuoteKeeper.GrantRole(ctx, ownerRole, owner); err != nil { - return err - } - upgradeRole, err := bridgeFeeQuoteKeeper.GetUpgradeRole(ctx) - if err != nil { - return err - } - if _, err := bridgeFeeQuoteKeeper.GrantRole(ctx, upgradeRole, owner); err != nil { - return err - } + if err := grantRoles(ctx, bridgeFeeQuoteKeeper, owner, bridgeFeeQuoteKeeper.GetOwnerRole, bridgeFeeQuoteKeeper.GetUpgradeRole); err != nil { + return err + } for _, bridgeDenom := range bridgeDenoms { if _, err := bridgeFeeQuoteKeeper.RegisterChain(ctx, bridgeDenom.ChainName, bridgeDenom.Denoms...); err != nil { return err } } return nil }Also applies to: 101-130
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (8)
app/app.go(4 hunks)app/upgrades/v8/upgrade.go(3 hunks)contract/bridge_fee.go(1 hunks)contract/bridge_fee_oracle.go(1 hunks)contract/bridge_fee_oracle.sol.go(1 hunks)contract/types.go(1 hunks)solidity/contracts/bridge/BridgeFeeOracle.sol(5 hunks)solidity/contracts/bridge/BridgeFeeQuote.sol(6 hunks)
✅ Files skipped from review due to trivial changes (1)
- contract/bridge_fee_oracle.sol.go
🚧 Files skipped from review as they are similar to previous changes (1)
- contract/bridge_fee_oracle.go
🔇 Additional comments (10)
app/upgrades/v8/upgrade.go (4)
5-5: LGTM: New imports are appropriate for bridge fee functionality.
The added imports support error handling and Ethereum address manipulation required for the bridge fee contract deployment.
Also applies to: 21-21, 26-26
78-79: LGTM: Keeper initialization is correct.
The bridge fee quote and oracle keepers are properly initialized with the required dependencies.
113-113: 🛠️ Refactor suggestion
Validate oracle selection logic.
Using oracles[0] without proper validation of its suitability could be risky. Consider implementing a more robust oracle selection mechanism.
- common.HexToAddress(oracles[0].ExternalAddress),
+ selectBestOracle(oracles), // Implement selectBestOracle function to choose the most reliable oracle99-102: Consider additional oracle validation.
The code only checks for the presence of oracles but doesn't validate their status or reliability. Consider adding more comprehensive validation.
- oracles := app.CrosschainKeepers.EthKeeper.GetAllOracles(cacheCtx, true)
- if oracles.Len() <= 0 {
- return fromVM, errors.New("no oracle found")
- }
+ oracles := app.CrosschainKeepers.EthKeeper.GetAllOracles(cacheCtx, true)
+ if oracles.Len() <= 0 {
+ return fromVM, errors.New("no oracle found")
+ }
+ // Validate oracle status and threshold
+ activeOracles := 0
+ for _, oracle := range oracles {
+ if oracle.Status {
+ activeOracles++
+ }
+ }
+ if activeOracles < requiredOracleThreshold {
+ return fromVM, fmt.Errorf("insufficient active oracles: %d, required: %d", activeOracles, requiredOracleThreshold)
+ }solidity/contracts/bridge/BridgeFeeQuote.sol (2)
5-5: LGTM: Access control upgrade
The switch from OwnableUpgradeable to AccessControlUpgradeable is a good improvement, providing more granular access control capabilities.
Also applies to: 28-28
68-74: Verify role management strategy
The current implementation grants all roles (OWNER_ROLE, UPGRADE_ROLE, DEFAULT_ADMIN_ROLE) to the contract deployer. Consider:
- Whether these roles should be separated in production
- Implementing a role renunciation mechanism
- Adding events for role changes
✅ Verification successful
Role management implementation is secure and follows best practices
The implementation already includes all necessary security features:
- Role separation is properly implemented with distinct roles (OWNER_ROLE, UPGRADE_ROLE, DEFAULT_ADMIN_ROLE)
- Role renunciation mechanism is available through the
renounceRolefunction - Role changes are tracked through events:
- RoleGranted
- RoleRevoked
- RoleAdminChanged
The initial granting of all roles to the deployer is a standard practice, allowing the deployer to later distribute roles to different addresses as needed in production.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Check for any other contracts that might be affected by this role change
rg -l "onlyOwner|Ownable" --type solidity
Length of output: 76
Script:
#!/bin/bash
# Let's try different approaches to analyze role management
# 1. Search for role-related patterns
rg -l "OWNER_ROLE|UPGRADE_ROLE|DEFAULT_ADMIN_ROLE"
# 2. Look for access control related imports and usage
rg -l "AccessControl|_grantRole|_revokeRole"
# 3. Search for role-related events
rg "event.*Role"
# 4. Check for role renunciation patterns
rg "renounce.*Role"
Length of output: 49341
app/app.go (3)
5-5: LGTM: Import changes are well-structured
The new imports are properly organized and necessary for the bridge fee contract deployment functionality.
Also applies to: 59-59, 69-69
351-352: 🛠️ Refactor suggestion
Add validation for keeper initialization
The keepers are initialized but their success is not validated. Consider adding error checks to ensure the keepers are properly initialized with valid addresses.
#!/bin/bash
# Verify the contract addresses are properly defined
rg -A 2 "BridgeFeeAddress|BridgeFeeOracleAddress" -bridgeFeeQuoteKeeper := contract.NewBridgeFeeQuoteKeeper(app.EvmKeeper, contract.BridgeFeeAddress)
-bridgeFeeOracleKeeper := contract.NewBridgeFeeOracleKeeper(app.EvmKeeper, contract.BridgeFeeOracleAddress)
+bridgeFeeQuoteKeeper, err := contract.NewBridgeFeeQuoteKeeper(app.EvmKeeper, contract.BridgeFeeAddress)
+if err != nil {
+ return nil, fmt.Errorf("failed to initialize bridge fee quote keeper: %w", err)
+}
+bridgeFeeOracleKeeper, err := contract.NewBridgeFeeOracleKeeper(app.EvmKeeper, contract.BridgeFeeOracleAddress)
+if err != nil {
+ return nil, fmt.Errorf("failed to initialize bridge fee oracle keeper: %w", err)
+}370-380: 🛠️ Refactor suggestion
Review bridge fee contract deployment parameters
Several concerns with the contract deployment:
- Using the same module address for both parameters might not be the intended configuration
- Using the first delegation address without validation could be risky
- Error message from deployment could be more descriptive
- if err = contract.DeployBridgeFeeContract(
+ if len(delegations[0].DelegatorAddress) == 0 {
+ return nil, errors.New("invalid delegator address: first delegation has empty address")
+ }
+ if err = contract.DeployBridgeFeeContract(
ctx,
app.EvmKeeper,
bridgeFeeQuoteKeeper,
bridgeFeeOracleKeeper,
bridgeDenoms,
moduleAddress, moduleAddress,
common.BytesToAddress(sdk.MustAccAddressFromBech32(delegations[0].DelegatorAddress).Bytes()),
); err != nil {
- return nil, err
+ return nil, fmt.Errorf("failed to deploy bridge fee contract: %w", err)
}solidity/contracts/bridge/BridgeFeeOracle.sol (1)
100-101: Access control update for _authorizeUpgrade is appropriate
Changing the access control of the _authorizeUpgrade function to require the UPGRADE_ROLE ensures that only authorized addresses can perform contract upgrades. This enhances the security of the upgrade mechanism.
| // TODO set bridge fee contract owner address before mainnet upgrade | ||
| moduleAddress, |
There was a problem hiding this comment.
Address TODO before mainnet deployment.
The TODO comment indicates that the bridge fee contract owner address needs to be set before mainnet upgrade. This is critical for security.
Please ensure this is addressed before deploying to mainnet. Consider:
- Using a dedicated admin/owner address
- Implementing a multi-sig wallet as the owner
- Setting up a governance-controlled owner
| function initialize(address _crosschain) public initializer { | ||
| crosschainContract = _crosschain; | ||
|
|
||
| __AccessControl_init(); | ||
| __UUPSUpgradeable_init(); | ||
| __ReentrancyGuard_init(); | ||
|
|
||
| crosschainContract = _crossChain; | ||
|
|
||
| _grantRole(DEFAULT_ADMIN_ROLE, msg.sender); | ||
| _grantRole(UPGRADE_ROLE, msg.sender); | ||
| _grantRole(OWNER_ROLE, msg.sender); |
There was a problem hiding this comment.
Call parent initializers before modifying state variables in initialize function
In the initialize function, you are setting crosschainContract before calling the parent initializers. Best practices recommend calling parent initializers before modifying any state variables to ensure that the contract is fully initialized and prevent potential issues related to uninitialized parent contracts.
Apply this diff to correct the initialization order:
function initialize(address _crosschain) public initializer {
+ __AccessControl_init();
+ __UUPSUpgradeable_init();
+ __ReentrancyGuard_init();
crosschainContract = _crosschain;
_grantRole(DEFAULT_ADMIN_ROLE, msg.sender);
_grantRole(UPGRADE_ROLE, msg.sender);
_grantRole(OWNER_ROLE, msg.sender);
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| function initialize(address _crosschain) public initializer { | |
| crosschainContract = _crosschain; | |
| __AccessControl_init(); | |
| __UUPSUpgradeable_init(); | |
| __ReentrancyGuard_init(); | |
| crosschainContract = _crossChain; | |
| _grantRole(DEFAULT_ADMIN_ROLE, msg.sender); | |
| _grantRole(UPGRADE_ROLE, msg.sender); | |
| _grantRole(OWNER_ROLE, msg.sender); | |
| function initialize(address _crosschain) public initializer { | |
| __AccessControl_init(); | |
| __UUPSUpgradeable_init(); | |
| __ReentrancyGuard_init(); | |
| crosschainContract = _crosschain; | |
| _grantRole(DEFAULT_ADMIN_ROLE, msg.sender); | |
| _grantRole(UPGRADE_ROLE, msg.sender); | |
| _grantRole(OWNER_ROLE, msg.sender); |
Summary by CodeRabbit
Release Notes
New Features
BridgeFeeOracle,BridgeFeeQuote, andBridgeProxy.OWNER_ROLEandUPGRADE_ROLEfor improved security.Bug Fixes
BridgeFeeQuoteKeeper.Documentation
Chores
compile.shscript.