feat(contracts-rfq): Multicall target abstraction [SLT-134] #3078
Conversation
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
| function multicallNoResults(bytes[] calldata data, bool ignoreReverts) external { | ||
| for (uint256 i = 0; i < data.length; ++i) { | ||
| // We perform a delegate call to ourself to preserve the msg.sender. This is identical to `msg.sender` | ||
| // calling the functions directly one by one, therefore doesn't add any security risks. | ||
| // Note: msg.value is also preserved when doing a delegate call, but this function is not payable, | ||
| // so it's always 0 and not a security risk. | ||
| (bool success, bytes memory result) = address(this).delegatecall(data[i]); | ||
| if (!success && !ignoreReverts) { | ||
| _bubbleRevert(result); | ||
| } | ||
| } | ||
| } |
Check notice
Code scanning / Slither
Calls inside a loop Low
| function multicallNoResults(bytes[] calldata data, bool ignoreReverts) external { | ||
| for (uint256 i = 0; i < data.length; ++i) { | ||
| // We perform a delegate call to ourself to preserve the msg.sender. This is identical to `msg.sender` | ||
| // calling the functions directly one by one, therefore doesn't add any security risks. | ||
| // Note: msg.value is also preserved when doing a delegate call, but this function is not payable, | ||
| // so it's always 0 and not a security risk. | ||
| (bool success, bytes memory result) = address(this).delegatecall(data[i]); | ||
| if (!success && !ignoreReverts) { | ||
| _bubbleRevert(result); | ||
| } | ||
| } | ||
| } |
Check warning
Code scanning / Slither
Low-level calls Warning
| function multicallWithResults( | ||
| bytes[] calldata data, | ||
| bool ignoreReverts | ||
| ) | ||
| external | ||
| returns (Result[] memory results) | ||
| { | ||
| results = new Result[](data.length); | ||
| for (uint256 i = 0; i < data.length; ++i) { | ||
| // We perform a delegate call to ourself to preserve the msg.sender. This is identical to `msg.sender` | ||
| // calling the functions directly one by one, therefore doesn't add any security risks. | ||
| // Note: msg.value is also preserved when doing a delegate call, but this function is not payable, | ||
| // so it's always 0 and not a security risk. | ||
| (results[i].success, results[i].returnData) = address(this).delegatecall(data[i]); | ||
| if (!results[i].success && !ignoreReverts) { | ||
| _bubbleRevert(results[i].returnData); | ||
| } | ||
| } | ||
| } |
Check notice
Code scanning / Slither
Calls inside a loop Low
| function multicallWithResults( | ||
| bytes[] calldata data, | ||
| bool ignoreReverts | ||
| ) | ||
| external | ||
| returns (Result[] memory results) | ||
| { | ||
| results = new Result[](data.length); | ||
| for (uint256 i = 0; i < data.length; ++i) { | ||
| // We perform a delegate call to ourself to preserve the msg.sender. This is identical to `msg.sender` | ||
| // calling the functions directly one by one, therefore doesn't add any security risks. | ||
| // Note: msg.value is also preserved when doing a delegate call, but this function is not payable, | ||
| // so it's always 0 and not a security risk. | ||
| (results[i].success, results[i].returnData) = address(this).delegatecall(data[i]); | ||
| if (!results[i].success && !ignoreReverts) { | ||
| _bubbleRevert(results[i].returnData); | ||
| } | ||
| } | ||
| } |
Check warning
Code scanning / Slither
Low-level calls Warning
| function _bubbleRevert(bytes memory returnData) internal pure { | ||
| // Look for revert reason and bubble it up if present | ||
| if (returnData.length > 0) { | ||
| // The easiest way to bubble the revert reason is using memory via assembly | ||
| // solhint-disable-next-line no-inline-assembly | ||
| assembly { | ||
| let returndata_size := mload(returnData) | ||
| revert(add(32, returnData), returndata_size) | ||
| } | ||
| } else { | ||
| revert MulticallTarget__UndeterminedRevert(); | ||
| } | ||
| } |
Check warning
Code scanning / Slither
Assembly usage Warning
WalkthroughThe recent changes introduce a new interface and contract for handling multiple calls in Ethereum smart contracts, enhancing batch processing capabilities. The Changes
Poem
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
Documentation and Community
|
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## master #3078 +/- ##
===================================================
+ Coverage 23.09876% 25.87753% +2.77877%
===================================================
Files 193 178 -15
Lines 11624 10484 -1140
Branches 82 119 +37
===================================================
+ Hits 2685 2713 +28
+ Misses 8697 7532 -1165
+ Partials 242 239 -3
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://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Files selected for processing (4)
- packages/contracts-rfq/contracts/interfaces/IMulticallTarget.sol (1 hunks)
- packages/contracts-rfq/contracts/utils/MulticallTarget.sol (1 hunks)
- packages/contracts-rfq/test/MulticallTarget.t.sol (1 hunks)
- packages/contracts-rfq/test/harnesses/MulticallTargetHarness.sol (1 hunks)
Additional context used
GitHub Check: Slither
packages/contracts-rfq/contracts/utils/MulticallTarget.sol
[notice] 19-30: Calls inside a loop
MulticallTarget.multicallNoResults(bytes[],bool) (contracts/utils/MulticallTarget.sol#19-30) has external calls inside a loop: (success,result) = address(this).delegatecall(data[i]) (contracts/utils/MulticallTarget.sol#25)
[warning] 19-30: Low-level calls
Low level call in MulticallTarget.multicallNoResults(bytes[],bool) (contracts/utils/MulticallTarget.sol#19-30):
- (success,result) = address(this).delegatecall(data[i]) (contracts/utils/MulticallTarget.sol#25)
[notice] 40-58: Calls inside a loop
MulticallTarget.multicallWithResults(bytes[],bool) (contracts/utils/MulticallTarget.sol#40-58) has external calls inside a loop: (results[i].success,results[i].returnData) = address(this).delegatecall(data[i]) (contracts/utils/MulticallTarget.sol#53)
[warning] 40-58: Low-level calls
Low level call in MulticallTarget.multicallWithResults(bytes[],bool) (contracts/utils/MulticallTarget.sol#40-58):
- (results[i].success,results[i].returnData) = address(this).delegatecall(data[i]) (contracts/utils/MulticallTarget.sol#53)
[warning] 63-75: Assembly usage
MulticallTarget._bubbleRevert(bytes) (contracts/utils/MulticallTarget.sol#63-75) uses assembly
- INLINE ASM (contracts/utils/MulticallTarget.sol#68-71)
Additional comments not posted (38)
packages/contracts-rfq/contracts/interfaces/IMulticallTarget.sol (4)
1-2: LGTM!The SPDX license identifier and pragma directive are correctly specified.
6-10: LGTM!The interface and struct are correctly defined.
12-12: LGTM!The function
multicallNoResultsis correctly defined.
13-18: LGTM!The function
multicallWithResultsis correctly defined.packages/contracts-rfq/test/harnesses/MulticallTargetHarness.sol (3)
1-2: LGTM!The SPDX license identifier and pragma directive are correctly specified.
4-4: LGTM!The import statement is correctly specified.
6-42: LGTM!The contract and its functions are correctly defined.
packages/contracts-rfq/contracts/utils/MulticallTarget.sol (3)
1-2: LGTM!The SPDX license identifier and pragma directive are correctly specified.
4-4: LGTM!The import statement is correctly specified.
6-75: LGTM!The contract and its functions are correctly defined.
Tools
GitHub Check: Slither
[notice] 19-30: Calls inside a loop
MulticallTarget.multicallNoResults(bytes[],bool) (contracts/utils/MulticallTarget.sol#19-30) has external calls inside a loop: (success,result) = address(this).delegatecall(data[i]) (contracts/utils/MulticallTarget.sol#25)
[warning] 19-30: Low-level calls
Low level call in MulticallTarget.multicallNoResults(bytes[],bool) (contracts/utils/MulticallTarget.sol#19-30):
- (success,result) = address(this).delegatecall(data[i]) (contracts/utils/MulticallTarget.sol#25)
[notice] 40-58: Calls inside a loop
MulticallTarget.multicallWithResults(bytes[],bool) (contracts/utils/MulticallTarget.sol#40-58) has external calls inside a loop: (results[i].success,results[i].returnData) = address(this).delegatecall(data[i]) (contracts/utils/MulticallTarget.sol#53)
[warning] 40-58: Low-level calls
Low level call in MulticallTarget.multicallWithResults(bytes[],bool) (contracts/utils/MulticallTarget.sol#40-58):
- (results[i].success,results[i].returnData) = address(this).delegatecall(data[i]) (contracts/utils/MulticallTarget.sol#53)
[warning] 63-75: Assembly usage
MulticallTarget._bubbleRevert(bytes) (contracts/utils/MulticallTarget.sol#63-75) uses assembly
- INLINE ASM (contracts/utils/MulticallTarget.sol#68-71)packages/contracts-rfq/test/MulticallTarget.t.sol (28)
1-2: LGTM!The SPDX license identifier and pragma directive are correct.
4-7: LGTM!The imports are necessary and correctly specified.
9-13: LGTM!The contract declaration and state variables are necessary for the test setup.
15-19: LGTM!The
setUpfunction correctly initializes the test environment.
21-23: LGTM!The
getEncodedStringRevertMessagefunction correctly encodes the string revert message.
25-32: LGTM!The
getMsgSenderDatafunction correctly encodes the function calls and returns them as an array.
34-41: LGTM!The
getMsgSenderResultsfunction correctly creates and returns the array of results.
43-50: LGTM!The
getNoRevertsDatafunction correctly encodes the function calls and returns them as an array.
52-59: LGTM!The
getNoRevertsResultsfunction correctly creates and returns the array of results.
61-67: LGTM!The
getCustomErrorRevertDatafunction correctly encodes the function calls and returns them as an array.
70-77: LGTM!The
getCustomErrorRevertResultsfunction correctly creates and returns the array of results.
79-85: LGTM!The
getStringRevertDatafunction correctly encodes the function calls and returns them as an array.
88-95: LGTM!The
getStringRevertResultsfunction correctly creates and returns the array of results.
97-103: LGTM!The
getUndeterminedRevertDatafunction correctly encodes the function calls and returns them as an array.
106-113: LGTM!The
getUndeterminedRevertResultsfunction correctly creates and returns the array of results.
117-123: LGTM!The
test_multicallNoResults_ignoreReverts_noRevertsfunction correctly tests themulticallNoResultsfunction.
125-132: LGTM!The
test_multicallNoResults_ignoreReverts_withMsgSenderfunction correctly tests themulticallNoResultsfunction.
134-140: LGTM!The
test_multicallNoResults_ignoreReverts_withCustomErrorRevertfunction correctly tests themulticallNoResultsfunction.
142-148: LGTM!The
test_multicallNoResults_ignoreReverts_withStringRevertfunction correctly tests themulticallNoResultsfunction.
150-156: LGTM!The
test_multicallNoResults_ignoreReverts_withUndeterminedRevertfunction correctly tests themulticallNoResultsfunction.
158-164: LGTM!The
test_multicallNoResults_dontIgnoreReverts_noRevertsfunction correctly tests themulticallNoResultsfunction.
166-173: LGTM!The
test_multicallNoResults_dontIgnoreReverts_withMsgSenderfunction correctly tests themulticallNoResultsfunction.
175-179: LGTM!The
test_multicallNoResults_dontIgnoreReverts_withCustomErrorRevertfunction correctly tests themulticallNoResultsfunction.
181-186: LGTM!The
test_multicallNoResults_dontIgnoreReverts_withStringRevertfunction correctly tests themulticallNoResultsfunction.
188-192: LGTM!The
test_multicallNoResults_dontIgnoreReverts_withUndeterminedRevertfunction correctly tests themulticallNoResultsfunction.
196-203: LGTM!The
test_multicallWithResults_ignoreReverts_noRevertsfunction correctly tests themulticallWithResultsfunction.
205-213: LGTM!The
test_multicallWithResults_ignoreReverts_withMsgSenderfunction correctly tests themulticallWithResultsfunction.
215-222: LGTM!The
test_multicallWithResults_ignoreReverts_withCustomErrorRevertfunction correctly tests themulticallWithResultsfunction.
ChiTimesChi
changed the title
Description
Adds an abstract contract that is capable of receiving the multicall (batched calls) instructions from any account, including the EOAs (which don't have the built-in multicall feature on the vast majority of EVM chains).
The contract has two point of entries:
multicallNoResultsfor doing batched calls when the returned value could be discarded. This is meant for EOAs to submit the transactions, as this saves gas and behaves identical tomulticallWithResultsoutside of not forwarding the returned values from the calls.multicallWithResultstransaction doesn't have any additional benefit.multicallWithResultsfor doing batched calls when the returned values are forwarded. This is meant for using viaeth_callto effectively simulate the transaction execution without publishing and obtaining the results (this could also include the view functions). This could be also used by another contracts on-chain, if they care about getting the returned values and don't have an alternative way do do a batched call.Metadata
Completes SLT-134
Summary by CodeRabbit
New Features
Tests