GMS-2051 Multi-caller v2 #241
Conversation
GMS-2051 Multi-caller v2
🚨 Report Summary
For more details view the full report in OpenZeppelin Code Inspector |
| revert EmptyCallArray(); | ||
| } | ||
| for (uint256 i = 0; i < _calls.length; i++) { | ||
| if (bytes(_calls[i].functionSignature).length == 0) { |
There was a problem hiding this comment.
Code coverage report indicates this branch isn't tested
There was a problem hiding this comment.
Note that I used the following, and then searched for GuardedMulticaller2:
forge coverage --report debug > x.txt
There was a problem hiding this comment.
I found the debug report not reliable. I added a test that should cover that line:
https://github.com/immutable/contracts/pull/241/files#diff-7891bd472e64eb648ea5f38d97917eacbfc82723eeb5beffe2c65ee58c4c6b30R268
calls[0] = GuardedMulticaller2.Call(address(target), "", abi.encodePacked(uint256(42)));
| if (bytes(_calls[i].functionSignature).length == 0) { | ||
| revert InvalidFunctionSignature(_calls[i]); | ||
| } | ||
| if (_calls[i].target.code.length == 0) { |
There was a problem hiding this comment.
Code coverage report indicates this branch isn't tested
There was a problem hiding this comment.
Similar to the comment above. There is a test that should cover that line:
https://github.com/immutable/contracts/pull/241/files#diff-7891bd472e64eb648ea5f38d97917eacbfc82723eeb5beffe2c65ee58c4c6b30R152
calls[0] = GuardedMulticaller2.Call(address(0), "succeedWithUint256(uint256)", abi.encodePacked(uint256(42)));
| // solhint-disable avoid-low-level-calls | ||
| // slither-disable-next-line calls-loop | ||
| (bool success, bytes memory returnData) = _calls[i].target.call(callData); | ||
| if (!success) { |
There was a problem hiding this comment.
I think the coverage report is indicating that this branch isn't being tested
There was a problem hiding this comment.
Similar to the comments above, there are two tests that should cover that line:
https://github.com/immutable/contracts/pull/241/files#diff-7891bd472e64eb648ea5f38d97917eacbfc82723eeb5beffe2c65ee58c4c6b30R249
https://github.com/immutable/contracts/pull/241/files#diff-7891bd472e64eb648ea5f38d97917eacbfc82723eeb5beffe2c65ee58c4c6b30R203
Thinking I'm missing something here
| (bool success, bytes memory returnData) = _calls[i].target.call(callData); | ||
| if (!success) { | ||
| // Look for revert reason and bubble it up if present | ||
| if (returnData.length == 0) { |
There was a problem hiding this comment.
If this return data length is less that four bytes, then the error returned is a non-valid ABI encoded revert result: that is, not a four byte function selector followed by encoded parameters. This would probably because the call was made to a contract that doesn't follow ABI encoding rules. Given this, the "revert(add(returnData, 32), mload(returnData))" will reference a memory location, likely never used thus far containing zeros.
I suggest changing to:
if (returnData.length < 4) {
There was a problem hiding this comment.
This logic is similar to the logic used by OpenZeppelin Address library. Not sure if this is something they failed to catch.
https://github.com/OpenZeppelin/openzeppelin-contracts/blob/master/contracts/utils/Address.sol#L138
function _revert(bytes memory returndata) private 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
/// @solidity memory-safe-assembly
assembly {
let returndata_size := mload(returndata)
revert(add(32, returndata), returndata_size)
}
} else {
revert Errors.FailedCall();
}
}There was a problem hiding this comment.
I have updated the logic to use 4 bytes for comparison and also extend FailedCall error to accept returnData as its param in case there is data returned from the function call and having less than 4 bytes.
|
One potential scenario that could be guarded against that isn't: The last account with DEFAULT_ADMIN_ROLE mistakenly calls renounceRole, thus leaving the contract with no account with DEFAULT_ADMIN_ROLE. The only reason for allowing this is so that the contract could become permissionless. However, I don't think any game studio would want this. |
# Conflicts: # abi/index.ts # wagmi.config.ts
@drinkcoffee I have updated our threat model with the following steps if such scenario happens:
|
This PR aims to improve the current implementation of Guarded Multi-caller
Regarding naming a new version, we follow
<Contract><VersionNumber>style. This style is adopted byCREATEfactory (CREATE,CREATE2, andCREATE3).Improve transaction's visibility with function signatures
In the v1 implementation,
calldata is a concatenation of a function selector and its arguments. This is done for efficiency and to align with multi-call standards. However, function selectors provide almost no information about what the functions do, which makes it very complex to understand the flow of a crafting transaction and what it does. We fix this issue by passing function signatures separately instead of encoding function selectors into the_dataparam right away.Use struct
Instead of separating
_targetsand_datainto different arrays, we can define a struct with one array for extensibility.Removal of function permits
https://immutable.atlassian.net/wiki/spaces/~6376e9423c26ca7fa0d17406/pages/2701230264/Multi-caller+Function+Permit+Removal
The function permits degrading developer experience while providing little security improvements. This PR removes the function permits.
Test Coverage