FE release 2024-03-29 #2400
FE release 2024-03-29 #2400
Conversation
Co-authored-by: Trajan0x <trajan0x@users.noreply.github.com>
* Install OZ contracts-upgradeable 5.0.1 * Scaffold SynapseExecutionServiceV1 * Port ExecService tests * Implement management * Implement fees/execution request * Implement ERC-7201 storage * Properly exclude proxy admin from fuzz tests * Update integration tests * Remove deprecated impl * Adjust configuration script * Adjust deployment workflow * Fix: config script * Chore: yarn install * Add harness for `SynapseExecutionServiceV1` * Update `ExecutionService.sol` references * Regenerate `sin-executor` * Chore: ExecutionService -> SynapseExecutionServiceV1Harness * Update ExecService deployment workflow in tests * Chore: implicit TransparentUpgradeableProxy deployment, docs * Chore: fix linter warnings * Regenerate * Mark SynExecServiceV1 functions as virtual to simplify V2 development * Implement global markup management * Chore: simplify Fees test * Add new tests * Implement global markup * Regenerate `sin-executor`
* Scaffold legacy message library * Implement * Scaffold legacy options library * Implement `LegacyOptionsLib` * Scaffold wrapper for Legacy MessageBus * Add interface for legacy receiver contracts * Expose nonce in MessageBus * Add unit tests for MessageBus * Implement management * Implement sending * Implement receiving, with some TODOs * Chore: fix linter warnings * Resolve TODOs, leave notes * Chore: consistent params ordering * Chore: silence linter in tests
) * Scaffold `payloadSize` library func * Add smol lib for math purposes * Implement `payloadSize` * Use `payloadSize()` instead of encoding in ClientV1 * Rework `client.getInterchainFee()` to take message len as param * Update Apps to use message length for getters * Chore: fix outdated terminology * Regenerate `sin-executor` * Chore: lint
Co-authored-by: Trajan0x <trajan0x@users.noreply.github.com>
* Fix: follow naming convention for deployed proxy * Fix: naming convention for ProxyAdmin (deployed by proxy)
* Feat: add GetChainIDsByStatus() to db service; fetch pending txs by chain * Feat: add TestGetChainIDsByStatus * [goreleaser] * Fix: mark ReplacedOrConfirmed on Pending / Stored statuses * [goreleaser]
* Fix: shadowing * Chore: silence linter warnings in scripts
* v3 SIN testnet deployment * Chore: rename to comply with #2392 * Chore: `forge fmt` --------- Co-authored-by: ChiTimesChi <88190723+ChiTimesChi@users.noreply.github.com>
abtestingalpha
requested review from
bigboydiamonds,
aureliusbtc,
trajan0x,
Defi-Moses and
ChiTimesChi
as code owners
github-actions
bot
added
M-ci
WalkthroughThis update involves enhancing a blockchain-related project with new features and improvements across its infrastructure. It introduces a function to retrieve chain IDs based on status, updates Docker tags for actions, and adds new chain IDs for various networks. The changes also include updates to contracts, scripts, and dependencies across several packages, aiming to improve functionality, compatibility, and security. Changes
Possibly related issues
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 as PR comments)
Additionally, you can add CodeRabbit Configration File (
|
![github-advanced-security[bot]](https://github.com/avatars/in/57789?s=60&v=4){kind=small}
| function getExecutionFee( | ||
| uint256 dstChainId, | ||
| uint256 txPayloadSize, | ||
| bytes memory options | ||
| ) | ||
| public | ||
| view | ||
| virtual | ||
| returns (uint256 executionFee) | ||
| { | ||
| address cachedGasOracle = gasOracle(); | ||
| if (cachedGasOracle == address(0)) { | ||
| revert SynapseExecutionService__GasOracleNotSet(); | ||
| } | ||
| // TODO: the "exact version" check should be generalized | ||
| (uint8 version,) = OptionsLib.decodeVersionedOptions(options); | ||
| if (version > OptionsLib.OPTIONS_V1) { | ||
| revert SynapseExecutionService__OptionsVersionNotSupported(version); | ||
| } | ||
| OptionsV1 memory optionsV1 = OptionsLib.decodeOptionsV1(options); | ||
| executionFee = IGasOracle(cachedGasOracle).estimateTxCostInLocalUnits({ | ||
| remoteChainId: dstChainId, | ||
| gasLimit: optionsV1.gasLimit, | ||
| calldataSize: txPayloadSize | ||
| }); | ||
| if (optionsV1.gasAirdrop > 0) { | ||
| executionFee += IGasOracle(cachedGasOracle).convertRemoteValueToLocalUnits({ | ||
| remoteChainId: dstChainId, | ||
| value: optionsV1.gasAirdrop | ||
| }); | ||
| } | ||
| executionFee += executionFee * globalMarkup() / WAD; | ||
| } |
Check warning
Code scanning / Slither
Unused return Medium
| function sendMessage( | ||
| bytes32 receiver, | ||
| uint256 dstChainId, | ||
| bytes calldata message, | ||
| bytes calldata options | ||
| ) | ||
| external | ||
| payable | ||
| { | ||
| address dstReceiver = TypeCasts.bytes32ToAddress(receiver); | ||
| if (TypeCasts.addressToBytes32(dstReceiver) != receiver) { | ||
| revert MessageBus__NotEVMReceiver(receiver); | ||
| } | ||
| uint64 cachedNonce = nonce++; | ||
| // Note: we are using the internal nonce here to generate the unique message ID. | ||
| // This is used for tracking purposes and is not used for replay protection. | ||
| // Instead, we rely on the Interchain Client to provide replay protection. | ||
| bytes memory encodedLegacyMsg = LegacyMessageLib.encodeLegacyMessage({ | ||
| srcSender: msg.sender, | ||
| dstReceiver: dstReceiver, | ||
| srcNonce: cachedNonce, | ||
| message: message | ||
| }); | ||
| _sendToLinkedApp({ | ||
| dstChainId: dstChainId, | ||
| messageFee: msg.value, | ||
| options: _icOptionsV1(options), | ||
| message: encodedLegacyMsg | ||
| }); | ||
| emit MessageSent({ | ||
| sender: msg.sender, | ||
| srcChainID: block.chainid, | ||
| receiver: receiver, | ||
| dstChainId: dstChainId, | ||
| message: message, | ||
| nonce: cachedNonce, | ||
| options: options, | ||
| fee: msg.value, | ||
| messageId: keccak256(encodedLegacyMsg) | ||
| }); | ||
| } |
Check notice
Code scanning / Slither
Reentrancy vulnerabilities Low
| function _receiveMessage( | ||
| uint256 srcChainId, | ||
| bytes32, // sender | ||
| uint256, // dbNonce | ||
| uint64, // entryIndex | ||
| bytes calldata encodedLegacyMsg | ||
| ) | ||
| internal | ||
| override | ||
| { | ||
| (address srcSender, address dstReceiver, uint64 srcNonce, bytes memory message) = | ||
| LegacyMessageLib.decodeLegacyMessage(encodedLegacyMsg); | ||
| // Note: we rely on the Interchain Client to provide replay protection. | ||
| ILegacyReceiver(dstReceiver).executeMessage({ | ||
| srcAddress: TypeCasts.addressToBytes32(srcSender), | ||
| srcChainId: srcChainId, | ||
| message: message, | ||
| executor: msg.sender | ||
| }); | ||
| emit Executed({ | ||
| messageId: keccak256(encodedLegacyMsg), | ||
| status: TxStatus.Success, | ||
| dstAddress: dstReceiver, | ||
| srcChainId: uint64(srcChainId), | ||
| srcNonce: srcNonce | ||
| }); | ||
| } |
Check notice
Code scanning / Slither
Reentrancy vulnerabilities Low
| function _getSynapseExecutionServiceV1Storage() private pure returns (SynapseExecutionServiceV1Storage storage $) { | ||
| // solhint-disable-next-line no-inline-assembly | ||
| assembly { | ||
| $.slot := SYNAPSE_EXECUTION_SERVICE_V1_STORAGE_LOCATION | ||
| } | ||
| } |
Check warning
Code scanning / Slither
Assembly usage Warning
| function encodeLegacyOptions(uint256 gasLimit) internal pure returns (bytes memory legacyOpts) { | ||
| return abi.encodePacked(LEGACY_OPTIONS_VERSION, gasLimit); | ||
| } |
Check warning
Code scanning / Slither
Dead-code Warning
| @@ -0,0 +1,30 @@ | |||
| // SPDX-License-Identifier: MIT | |||
| pragma solidity ^0.8.0; | |||
Check warning
Code scanning / Slither
Incorrect versions of Solidity Warning
| @@ -0,0 +1,53 @@ | |||
| // SPDX-License-Identifier: MIT | |||
| pragma solidity ^0.8.0; | |||
Check warning
Code scanning / Slither
Incorrect versions of Solidity Warning
| @@ -0,0 +1,28 @@ | |||
| // SPDX-License-Identifier: MIT | |||
| pragma solidity ^0.8.0; | |||
Check warning
Code scanning / Slither
Incorrect versions of Solidity Warning
| @@ -0,0 +1,119 @@ | |||
| // SPDX-License-Identifier: MIT | |||
| pragma solidity 0.8.20; | |||
Check warning
Code scanning / Slither
Incorrect versions of Solidity Warning
| /// @dev The length of the legacy options format: 2 bytes for the version and 32 bytes for the gas limit. | ||
| uint256 private constant LEGACY_OPTIONS_LENGTH = 34; | ||
| /// @dev The offset of the gas limit in the legacy options format. | ||
| uint256 private constant GAS_LIMIT_OFFSET = 2; |
Check warning
Code scanning / Slither
Unused state variable Warning
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## fe-release #2400 +/- ##
====================================================
+ Coverage 47.18914% 49.13394% +1.94480%
====================================================
Files 417 464 +47
Lines 29973 31522 +1549
Branches 220 509 +289
====================================================
+ Hits 14144 15488 +1344
- Misses 14357 14488 +131
- Partials 1472 1546 +74
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. |
![coderabbitai[bot]](https://github.com/avatars/in/347564?s=60&v=4){kind=small}
| # [0.4.0](https://github.com/synapsecns/sanguine/compare/@synapsecns/sdk-router@0.3.29...@synapsecns/sdk-router@0.4.0) (2024-03-29) | ||
|
|
||
|
|
||
| ### Features | ||
|
|
||
| * **sdk-router:** Add Base to the list of supported RFQ chains ([#2398](https://github.com/synapsecns/sanguine/issues/2398)) ([0425595](https://github.com/synapsecns/sanguine/commit/0425595c026bbc698a5f8a9ffd8f500fd2db1ae9)) |
There was a problem hiding this comment.
The changelog update for version 0.4.0 correctly documents the addition of "Base" to the list of supported RFQ chains. However, there's a minor typographical issue detected in the context around the feature description. It's generally a good practice to ensure all textual content is free from spelling mistakes to maintain professionalism in documentation.
-* **sdk-router:** Add Base to the list of supported RFQ chains ([#2398](https://github.com/synapsecns/sanguine/issues/2398)) ([0425595](https://github.com/synapsecns/sanguine/commit/0425595c026bbc698a5f8a9ffd8f500fd2db1ae9))
+* **sdk-router:** Added Base to the list of supported RFQ chains ([#2398](https://github.com/synapsecns/sanguine/issues/2398)) ([0425595](https://github.com/synapsecns/sanguine/commit/0425595c026bbc698a5f8a9ffd8f500fd2db1ae9))While the original text is not incorrect, adjusting the verb tense to "Added" aligns better with the conventional past tense used in changelogs for completed actions.
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.
| # [0.4.0](https://github.com/synapsecns/sanguine/compare/@synapsecns/sdk-router@0.3.29...@synapsecns/sdk-router@0.4.0) (2024-03-29) | |
| ### Features | |
| * **sdk-router:** Add Base to the list of supported RFQ chains ([#2398](https://github.com/synapsecns/sanguine/issues/2398)) ([0425595](https://github.com/synapsecns/sanguine/commit/0425595c026bbc698a5f8a9ffd8f500fd2db1ae9)) | |
| # [0.4.0](https://github.com/synapsecns/sanguine/compare/@synapsecns/sdk-router@0.3.29...@synapsecns/sdk-router@0.4.0) (2024-03-29) | |
| ### Features | |
| * **sdk-router:** Added Base to the list of supported RFQ chains ([#2398](https://github.com/synapsecns/sanguine/issues/2398)) ([0425595](https://github.com/synapsecns/sanguine/commit/0425595c026bbc698a5f8a9ffd8f500fd2db1ae9)) |
| @@ -0,0 +1,6 @@ | |||
| // SPDX-License-Identifier: MIT | |||
| pragma solidity ^0.8.0; | |||
There was a problem hiding this comment.
The pragma directive pragma solidity ^0.8.0; is too broad, potentially allowing for compilation with older, less secure versions of Solidity. Consider specifying a more precise version range or the latest version to ensure compatibility and security.
- pragma solidity ^0.8.0;
+ pragma solidity 0.8.20;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.
| pragma solidity ^0.8.0; | |
| pragma solidity 0.8.20; |
| @@ -0,0 +1,10 @@ | |||
| // SPDX-License-Identifier: MIT | |||
| pragma solidity ^0.8.0; | |||
There was a problem hiding this comment.
The pragma directive pragma solidity ^0.8.0; is too broad, potentially allowing for compilation with older, less secure versions of Solidity. Consider specifying a more precise version range or the latest version to ensure compatibility and security.
- pragma solidity ^0.8.0;
+ pragma solidity 0.8.20;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.
| pragma solidity ^0.8.0; | |
| pragma solidity 0.8.20; |
| @@ -0,0 +1,13 @@ | |||
| // SPDX-License-Identifier: MIT | |||
| pragma solidity ^0.8.0; | |||
There was a problem hiding this comment.
The pragma directive pragma solidity ^0.8.0; is too broad, potentially allowing for compilation with older, less secure versions of Solidity. Consider specifying a more precise version range or the latest version to ensure compatibility and security.
- pragma solidity ^0.8.0;
+ pragma solidity 0.8.20;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.
| pragma solidity ^0.8.0; | |
| pragma solidity 0.8.20; |
| @@ -0,0 +1,28 @@ | |||
| // SPDX-License-Identifier: MIT | |||
| pragma solidity ^0.8.0; | |||
There was a problem hiding this comment.
The pragma directive pragma solidity ^0.8.0; is too broad, potentially allowing for compilation with older, less secure versions of Solidity. Consider specifying a more precise version range or the latest version to ensure compatibility and security.
- pragma solidity ^0.8.0;
+ pragma solidity 0.8.20;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.
| pragma solidity ^0.8.0; | |
| pragma solidity 0.8.20; |
| // GetChainIDsByStatus returns the distinct chain ids for a given address and status. | ||
| func (s *Store) GetChainIDsByStatus(ctx context.Context, fromAddress common.Address, matchStatuses ...db.Status) (chainIDs []*big.Int, err error) { | ||
| chainIDs64 := []uint64{} | ||
|
|
||
| inArgs := statusToArgs(matchStatuses...) | ||
|
|
||
| query := ETHTX{ | ||
| From: fromAddress.String(), | ||
| } | ||
|
|
||
| tx := s.DB().WithContext(ctx). | ||
| Model(ÐTX{}). | ||
| Select(chainIDFieldName). | ||
| Distinct(). | ||
| Where(query). | ||
| Where(fmt.Sprintf("%s IN ?", statusFieldName), inArgs). | ||
| Find(&chainIDs64) | ||
| if tx.Error != nil { | ||
| return nil, fmt.Errorf("could not get chain ids: %w", tx.Error) | ||
| } | ||
|
|
||
| for _, chainID64 := range chainIDs64 { | ||
| chainIDs = append(chainIDs, new(big.Int).SetUint64(chainID64)) | ||
| } | ||
|
|
||
| return chainIDs, nil | ||
| } |
There was a problem hiding this comment.
The addition of the GetChainIDsByStatus function is a significant enhancement. It introduces a new capability to retrieve distinct chain IDs for a given address and status, which can be crucial for operations that need to process transactions based on their blockchain and status. Ensure that this function is thoroughly tested, especially for edge cases such as no matching records or handling of multiple statuses.
Would you like me to help with generating unit tests for the GetChainIDsByStatus function?
| import {OptionsLib, OptionsV1} from "../../contracts/libs/Options.sol"; | ||
|
|
||
| import {SynapseExecutionServiceV1Test} from "./SynapseExecutionServiceV1.t.sol"; | ||
|
|
||
| import {SynapseGasOracleMock} from "../mocks/SynapseGasOracleMock.sol"; |
There was a problem hiding this comment.
Consider grouping imports for better readability and maintainability.
-import {OptionsLib, OptionsV1} from "../../contracts/libs/Options.sol";
-import {SynapseExecutionServiceV1Test} from "./SynapseExecutionServiceV1.t.sol";
-import {SynapseGasOracleMock} from "../mocks/SynapseGasOracleMock.sol";
+import {
+ OptionsLib,
+ OptionsV1
+} from "../../contracts/libs/Options.sol";
+import { SynapseExecutionServiceV1Test } from "./SynapseExecutionServiceV1.t.sol";
+import { SynapseGasOracleMock } from "../mocks/SynapseGasOracleMock.sol";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.
| import {OptionsLib, OptionsV1} from "../../contracts/libs/Options.sol"; | |
| import {SynapseExecutionServiceV1Test} from "./SynapseExecutionServiceV1.t.sol"; | |
| import {SynapseGasOracleMock} from "../mocks/SynapseGasOracleMock.sol"; | |
| import { | |
| OptionsLib, | |
| OptionsV1 | |
| } from "../../contracts/libs/Options.sol"; | |
| import { SynapseExecutionServiceV1Test } from "./SynapseExecutionServiceV1.t.sol"; | |
| import { SynapseGasOracleMock } from "../mocks/SynapseGasOracleMock.sol"; |
| function test_getExecutionFee_noAirdrop() public { | ||
| uint256 fee = service.getExecutionFee({ | ||
| dstChainId: REMOTE_CHAIN_ID, | ||
| txPayloadSize: MOCK_CALLDATA_SIZE, | ||
| options: optionsNoAirdrop.encodeOptionsV1() | ||
| }); | ||
| assertEq(fee, MOCK_FEE_NO_AIRDROP); |
There was a problem hiding this comment.
The test test_getExecutionFee_noAirdrop correctly asserts the execution fee without airdrop. Ensure comprehensive coverage by adding tests for edge cases and unexpected inputs.
Would you like me to help generate additional test cases for edge conditions?
| function test_requestExecution_clientA_noAirdrop_exactFee() public { | ||
| expectEventExecutionRequested(MOCK_TX_ID, icClientA); | ||
| requestExecution(icClientA, MOCK_FEE_NO_AIRDROP, encodedOptionsNoAirdrop); | ||
| } | ||
|
|
||
| function test_requestExecution_clientA_noAirdrop_higherFee() public { | ||
| expectEventExecutionRequested(MOCK_TX_ID, icClientA); | ||
| requestExecution(icClientA, MOCK_FEE_NO_AIRDROP + 1, encodedOptionsNoAirdrop); | ||
| } | ||
|
|
||
| function test_requestExecution_clientA_noAirdrop_revert_lowerFee() public { | ||
| expectRevertFeeAmountTooLow(MOCK_FEE_NO_AIRDROP - 1, MOCK_FEE_NO_AIRDROP); | ||
| requestExecution(icClientA, MOCK_FEE_NO_AIRDROP - 1, encodedOptionsNoAirdrop); | ||
| } |
There was a problem hiding this comment.
The tests for requestExecution with exact, higher, and lower fees are well-structured. However, consider adding tests for boundary values of the fee amount to ensure robustness.
Would you like assistance in creating boundary value tests for the fee amount?
| @@ -0,0 +1,30 @@ | |||
| // SPDX-License-Identifier: MIT | |||
| pragma solidity ^0.8.0; | |||
There was a problem hiding this comment.
The pragma directive ^0.8.0 allows for older, potentially less secure versions of Solidity. Consider specifying a more recent version, such as ^0.8.20, to ensure the use of up-to-date compiler features and security fixes.
- pragma solidity ^0.8.0;
+ pragma solidity ^0.8.20;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.
| pragma solidity ^0.8.0; | |
| pragma solidity ^0.8.20; |
| @@ -0,0 +1,53 @@ | |||
| // SPDX-License-Identifier: MIT | |||
| pragma solidity ^0.8.0; | |||
There was a problem hiding this comment.
The pragma directive ^0.8.0 allows older, potentially insecure versions of Solidity. Consider specifying a more recent or fixed version, such as ^0.8.20, for improved security and compatibility.
- pragma solidity ^0.8.0;
+ pragma solidity ^0.8.20;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.
| pragma solidity ^0.8.0; | |
| pragma solidity ^0.8.20; |
| function setGovernor() internal { | ||
| printLog("Setting Governor"); | ||
| bytes32 governorRole = service.GOVERNOR_ROLE(); | ||
| if (!service.hasRole(governorRole, msg.sender)) { | ||
| service.grantRole(governorRole, msg.sender); | ||
| printSuccessWithIndent(string.concat("Granted Governor role to ", vm.toString(msg.sender))); | ||
| } else { | ||
| printSkipWithIndent(string.concat("governor role already granted to ", vm.toString(msg.sender))); | ||
| } |
There was a problem hiding this comment.
The setGovernor function correctly checks for the governor role before granting it, which is a secure practice. However, ensure that test coverage is added for this function to verify its behavior under various conditions.
Would you like me to help with adding test coverage for this function?
| @@ -0,0 +1,37 @@ | |||
| // SPDX-License-Identifier: MIT | |||
| pragma solidity ^0.8.0; | |||
There was a problem hiding this comment.
The pragma directive ^0.8.0 allows for older, potentially less secure versions of Solidity. Consider specifying a more recent version, such as ^0.8.20, to ensure the use of up-to-date compiler features and security fixes.
- pragma solidity ^0.8.0;
+ pragma solidity ^0.8.20;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.
| pragma solidity ^0.8.0; | |
| pragma solidity ^0.8.20; |
| @@ -0,0 +1,34 @@ | |||
| // SPDX-License-Identifier: MIT | |||
| pragma solidity ^0.8.0; | |||
There was a problem hiding this comment.
The pragma directive ^0.8.0 allows for older, potentially less secure versions of Solidity. Consider specifying a more recent version, such as ^0.8.20, to ensure the use of up-to-date compiler features and security fixes.
- pragma solidity ^0.8.0;
+ pragma solidity ^0.8.20;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.
| pragma solidity ^0.8.0; | |
| pragma solidity ^0.8.20; |
| @@ -51,7 +51,7 @@ | |||
| view | |||
| returns (uint256) | |||
| { | |||
| return _getMessageFee(dstChainId, OptionsV1({gasLimit: gasLimit, gasAirdrop: gasAirdrop}), message); | |||
| return _getMessageFee(dstChainId, OptionsV1({gasLimit: gasLimit, gasAirdrop: gasAirdrop}), message.length); | |||
There was a problem hiding this comment.
The update to pass message.length to _getMessageFee in the getMessageFee function is consistent with the shift towards using message lengths for fee calculations. However, this line was flagged by static analysis as not covered by tests. Ensure that there are tests covering this change to validate its correctness and to prevent regressions in the future.
+ // TODO: Add test coverage for getMessageFee with message.lengthCommittable 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.
| return _getMessageFee(dstChainId, OptionsV1({gasLimit: gasLimit, gasAirdrop: gasAirdrop}), message.length); | |
| // TODO: Add test coverage for getMessageFee with message.length | |
| return _getMessageFee(dstChainId, OptionsV1({gasLimit: gasLimit, gasAirdrop: gasAirdrop}), message.length); |
| function sendMessage( | ||
| bytes32 receiver, | ||
| uint256 dstChainId, | ||
| bytes calldata message, | ||
| bytes calldata options | ||
| ) | ||
| external | ||
| payable | ||
| { | ||
| address dstReceiver = TypeCasts.bytes32ToAddress(receiver); | ||
| if (TypeCasts.addressToBytes32(dstReceiver) != receiver) { | ||
| revert MessageBus__NotEVMReceiver(receiver); | ||
| } | ||
| uint64 cachedNonce = nonce++; | ||
| // Note: we are using the internal nonce here to generate the unique message ID. | ||
| // This is used for tracking purposes and is not used for replay protection. | ||
| // Instead, we rely on the Interchain Client to provide replay protection. | ||
| bytes memory encodedLegacyMsg = LegacyMessageLib.encodeLegacyMessage({ | ||
| srcSender: msg.sender, | ||
| dstReceiver: dstReceiver, | ||
| srcNonce: cachedNonce, | ||
| message: message | ||
| }); | ||
| _sendToLinkedApp({ | ||
| dstChainId: dstChainId, | ||
| messageFee: msg.value, | ||
| options: _icOptionsV1(options), | ||
| message: encodedLegacyMsg | ||
| }); | ||
| emit MessageSent({ | ||
| sender: msg.sender, | ||
| srcChainID: block.chainid, | ||
| receiver: receiver, | ||
| dstChainId: dstChainId, | ||
| message: message, | ||
| nonce: cachedNonce, | ||
| options: options, | ||
| fee: msg.value, | ||
| messageId: keccak256(encodedLegacyMsg) | ||
| }); | ||
| } |
There was a problem hiding this comment.
The sendMessage function in the MessageBus contract correctly implements the logic for sending messages across chains. However, the static analysis tool flagged a potential reentrancy vulnerability due to the external call in _sendToLinkedApp followed by an event emission.
Consider using the Checks-Effects-Interactions pattern to mitigate potential reentrancy attacks. Ensure that all state changes are completed before calling external contracts and emitting events.
- _sendToLinkedApp({
+ emit MessageSent({
+ sender: msg.sender,
+ srcChainID: block.chainid,
+ receiver: receiver,
+ dstChainId: dstChainId,
+ message: message,
+ nonce: cachedNonce,
+ options: options,
+ fee: msg.value,
+ messageId: keccak256(encodedLegacyMsg)
+ });
+ _sendToLinkedApp({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.
| function sendMessage( | |
| bytes32 receiver, | |
| uint256 dstChainId, | |
| bytes calldata message, | |
| bytes calldata options | |
| ) | |
| external | |
| payable | |
| { | |
| address dstReceiver = TypeCasts.bytes32ToAddress(receiver); | |
| if (TypeCasts.addressToBytes32(dstReceiver) != receiver) { | |
| revert MessageBus__NotEVMReceiver(receiver); | |
| } | |
| uint64 cachedNonce = nonce++; | |
| // Note: we are using the internal nonce here to generate the unique message ID. | |
| // This is used for tracking purposes and is not used for replay protection. | |
| // Instead, we rely on the Interchain Client to provide replay protection. | |
| bytes memory encodedLegacyMsg = LegacyMessageLib.encodeLegacyMessage({ | |
| srcSender: msg.sender, | |
| dstReceiver: dstReceiver, | |
| srcNonce: cachedNonce, | |
| message: message | |
| }); | |
| _sendToLinkedApp({ | |
| dstChainId: dstChainId, | |
| messageFee: msg.value, | |
| options: _icOptionsV1(options), | |
| message: encodedLegacyMsg | |
| }); | |
| emit MessageSent({ | |
| sender: msg.sender, | |
| srcChainID: block.chainid, | |
| receiver: receiver, | |
| dstChainId: dstChainId, | |
| message: message, | |
| nonce: cachedNonce, | |
| options: options, | |
| fee: msg.value, | |
| messageId: keccak256(encodedLegacyMsg) | |
| }); | |
| } | |
| function sendMessage( | |
| bytes32 receiver, | |
| uint256 dstChainId, | |
| bytes calldata message, | |
| bytes calldata options | |
| ) | |
| external | |
| payable | |
| { | |
| address dstReceiver = TypeCasts.bytes32ToAddress(receiver); | |
| if (TypeCasts.addressToBytes32(dstReceiver) != receiver) { | |
| revert MessageBus__NotEVMReceiver(receiver); | |
| } | |
| uint64 cachedNonce = nonce++; | |
| // Note: we are using the internal nonce here to generate the unique message ID. | |
| // This is used for tracking purposes and is not used for replay protection. | |
| // Instead, we rely on the Interchain Client to provide replay protection. | |
| bytes memory encodedLegacyMsg = LegacyMessageLib.encodeLegacyMessage({ | |
| srcSender: msg.sender, | |
| dstReceiver: dstReceiver, | |
| srcNonce: cachedNonce, | |
| message: message | |
| }); | |
| emit MessageSent({ | |
| sender: msg.sender, | |
| srcChainID: block.chainid, | |
| receiver: receiver, | |
| dstChainId: dstChainId, | |
| message: message, | |
| nonce: cachedNonce, | |
| options: options, | |
| fee: msg.value, | |
| messageId: keccak256(encodedLegacyMsg) | |
| }); | |
| _sendToLinkedApp({ | |
| dstChainId: dstChainId, | |
| messageFee: msg.value, | |
| options: _icOptionsV1(options), | |
| message: encodedLegacyMsg | |
| }); | |
| } |
| function _receiveMessage( | ||
| uint256 srcChainId, | ||
| bytes32, // sender | ||
| uint256, // dbNonce | ||
| uint64, // entryIndex | ||
| bytes calldata encodedLegacyMsg | ||
| ) | ||
| internal | ||
| override | ||
| { | ||
| (address srcSender, address dstReceiver, uint64 srcNonce, bytes memory message) = | ||
| LegacyMessageLib.decodeLegacyMessage(encodedLegacyMsg); | ||
| // Note: we rely on the Interchain Client to provide replay protection. | ||
| ILegacyReceiver(dstReceiver).executeMessage({ | ||
| srcAddress: TypeCasts.addressToBytes32(srcSender), | ||
| srcChainId: srcChainId, | ||
| message: message, | ||
| executor: msg.sender | ||
| }); | ||
| emit Executed({ | ||
| messageId: keccak256(encodedLegacyMsg), | ||
| status: TxStatus.Success, | ||
| dstAddress: dstReceiver, | ||
| srcChainId: uint64(srcChainId), | ||
| srcNonce: srcNonce | ||
| }); | ||
| } |
There was a problem hiding this comment.
The _receiveMessage function handles the receipt of messages, invoking the executeMessage function on the destination receiver. The static analysis tool flagged a potential reentrancy vulnerability here as well.
To mitigate potential reentrancy attacks in _receiveMessage, ensure that all state changes and validations occur before making external calls.
Consider reordering operations or using mutexes to prevent reentrancy if state changes are required after the external call.
| @@ -0,0 +1,119 @@ | |||
| // SPDX-License-Identifier: MIT | |||
| pragma solidity 0.8.20; | |||
There was a problem hiding this comment.
The Solidity version specified (0.8.20) is flagged as too recent to be trusted. While newer versions often include important fixes and improvements, they might also introduce new, undiscovered issues.
Consider using a more widely adopted and tested version of Solidity, such as 0.8.18, unless specific features or fixes from 0.8.20 are required for this contract.
- pragma solidity 0.8.20;
+ pragma solidity 0.8.18;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.
| pragma solidity 0.8.20; | |
| pragma solidity 0.8.18; |
| function getExecutionFee( | ||
| uint256 dstChainId, | ||
| uint256 txPayloadSize, | ||
| bytes memory options | ||
| ) | ||
| public | ||
| view | ||
| virtual | ||
| returns (uint256 executionFee) | ||
| { | ||
| address cachedGasOracle = gasOracle(); | ||
| if (cachedGasOracle == address(0)) { | ||
| revert SynapseExecutionService__GasOracleNotSet(); | ||
| } | ||
| // TODO: the "exact version" check should be generalized | ||
| (uint8 version,) = OptionsLib.decodeVersionedOptions(options); | ||
| if (version > OptionsLib.OPTIONS_V1) { | ||
| revert SynapseExecutionService__OptionsVersionNotSupported(version); | ||
| } | ||
| OptionsV1 memory optionsV1 = OptionsLib.decodeOptionsV1(options); | ||
| executionFee = IGasOracle(cachedGasOracle).estimateTxCostInLocalUnits({ | ||
| remoteChainId: dstChainId, | ||
| gasLimit: optionsV1.gasLimit, | ||
| calldataSize: txPayloadSize | ||
| }); | ||
| if (optionsV1.gasAirdrop > 0) { | ||
| executionFee += IGasOracle(cachedGasOracle).convertRemoteValueToLocalUnits({ | ||
| remoteChainId: dstChainId, | ||
| value: optionsV1.gasAirdrop | ||
| }); | ||
| } | ||
| executionFee += executionFee * globalMarkup() / WAD; | ||
| } |
There was a problem hiding this comment.
The getExecutionFee function calculates the execution fee based on the destination chain ID, transaction payload size, and options. However, the static analysis tool flagged an unused return value in the decodeVersionedOptions call.
Ensure that all returned values from function calls are used appropriately or explicitly ignored if not needed. This helps in avoiding potential bugs and makes the code more readable.
- (uint8 version,) = OptionsLib.decodeVersionedOptions(options);
+ (uint8 version, ) = OptionsLib.decodeVersionedOptions(options);
+ require(version <= OptionsLib.OPTIONS_V1, "Unsupported options version");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.
| function getExecutionFee( | |
| uint256 dstChainId, | |
| uint256 txPayloadSize, | |
| bytes memory options | |
| ) | |
| public | |
| view | |
| virtual | |
| returns (uint256 executionFee) | |
| { | |
| address cachedGasOracle = gasOracle(); | |
| if (cachedGasOracle == address(0)) { | |
| revert SynapseExecutionService__GasOracleNotSet(); | |
| } | |
| // TODO: the "exact version" check should be generalized | |
| (uint8 version,) = OptionsLib.decodeVersionedOptions(options); | |
| if (version > OptionsLib.OPTIONS_V1) { | |
| revert SynapseExecutionService__OptionsVersionNotSupported(version); | |
| } | |
| OptionsV1 memory optionsV1 = OptionsLib.decodeOptionsV1(options); | |
| executionFee = IGasOracle(cachedGasOracle).estimateTxCostInLocalUnits({ | |
| remoteChainId: dstChainId, | |
| gasLimit: optionsV1.gasLimit, | |
| calldataSize: txPayloadSize | |
| }); | |
| if (optionsV1.gasAirdrop > 0) { | |
| executionFee += IGasOracle(cachedGasOracle).convertRemoteValueToLocalUnits({ | |
| remoteChainId: dstChainId, | |
| value: optionsV1.gasAirdrop | |
| }); | |
| } | |
| executionFee += executionFee * globalMarkup() / WAD; | |
| } | |
| function getExecutionFee( | |
| uint256 dstChainId, | |
| uint256 txPayloadSize, | |
| bytes memory options | |
| ) | |
| public | |
| view | |
| virtual | |
| returns (uint256 executionFee) | |
| { | |
| address cachedGasOracle = gasOracle(); | |
| if (cachedGasOracle == address(0)) { | |
| revert SynapseExecutionService__GasOracleNotSet(); | |
| } | |
| // TODO: the "exact version" check should be generalized | |
| (uint8 version, ) = OptionsLib.decodeVersionedOptions(options); | |
| require(version <= OptionsLib.OPTIONS_V1, "Unsupported options version"); | |
| OptionsV1 memory optionsV1 = OptionsLib.decodeOptionsV1(options); | |
| executionFee = IGasOracle(cachedGasOracle).estimateTxCostInLocalUnits({ | |
| remoteChainId: dstChainId, | |
| gasLimit: optionsV1.gasLimit, | |
| calldataSize: txPayloadSize | |
| }); | |
| if (optionsV1.gasAirdrop > 0) { | |
| executionFee += IGasOracle(cachedGasOracle).convertRemoteValueToLocalUnits({ | |
| remoteChainId: dstChainId, | |
| value: optionsV1.gasAirdrop | |
| }); | |
| } | |
| executionFee += executionFee * globalMarkup() / WAD; | |
| } |
| function payloadSize(uint256 optionsLen, uint256 messageLen) internal pure returns (uint256) { | ||
| // 8 fields * 32 bytes (6 values for static, 2 offsets for dynamic) + 2 * 32 bytes (lengths for dynamic) = 320 | ||
| // abi.encode() also prepends the global offset (which is always 0x20) if there's a dynamic field, making it 352 | ||
| // Both options and message are dynamic fields, which are padded up to 32 bytes | ||
| return 352 + optionsLen.roundUpToWord() + messageLen.roundUpToWord(); | ||
| } |
There was a problem hiding this comment.
The new payloadSize function provides a way to calculate the payload size of an interchain transaction. This is a critical addition for accurately estimating transaction costs. Ensure that this function is covered by unit tests to validate its correctness across a range of input values.
+ // TODO: Add unit tests for payloadSize functionCommittable 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.
| function payloadSize(uint256 optionsLen, uint256 messageLen) internal pure returns (uint256) { | |
| // 8 fields * 32 bytes (6 values for static, 2 offsets for dynamic) + 2 * 32 bytes (lengths for dynamic) = 320 | |
| // abi.encode() also prepends the global offset (which is always 0x20) if there's a dynamic field, making it 352 | |
| // Both options and message are dynamic fields, which are padded up to 32 bytes | |
| return 352 + optionsLen.roundUpToWord() + messageLen.roundUpToWord(); | |
| } | |
| function payloadSize(uint256 optionsLen, uint256 messageLen) internal pure returns (uint256) { | |
| // 8 fields * 32 bytes (6 values for static, 2 offsets for dynamic) + 2 * 32 bytes (lengths for dynamic) = 320 | |
| // abi.encode() also prepends the global offset (which is always 0x20) if there's a dynamic field, making it 352 | |
| // Both options and message are dynamic fields, which are padded up to 32 bytes | |
| // TODO: Add unit tests for payloadSize function | |
| return 352 + optionsLen.roundUpToWord() + messageLen.roundUpToWord(); | |
| } |
| @@ -0,0 +1,146 @@ | |||
| // SPDX-License-Identifier: MIT | |||
| pragma solidity 0.8.20; | |||
There was a problem hiding this comment.
The Solidity version specified (0.8.20) is flagged as too recent to be trusted. While newer versions often include important fixes and improvements, they might also introduce new, undiscovered issues.
Consider using a more widely adopted and tested version of Solidity, such as 0.8.18, unless specific features or fixes from 0.8.20 are required for this contract.
- pragma solidity 0.8.20;
+ pragma solidity 0.8.18;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.
| pragma solidity 0.8.20; | |
| pragma solidity 0.8.18; |
null
4172612d475723b7198f4c5e76de21a3d27a1113: synapse-interface preview link
Summary by CodeRabbit
executionservicepackage for clarity and maintainability.flatten.shscripts, facilitating easier contract management and deployment.