immutable / contracts

Smart contracts for developers in the Immutable ecosystem.
Other
36 stars 36 forks source link

GMS-2051 Multi-caller v2 #241

Closed hiep-immutable closed 2 months ago

hiep-immutable commented 2 months ago

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 by CREATE factory (CREATE, CREATE2, and CREATE3).

Improve transaction's visibility with function signatures

In the v1 implementation, call data 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 _data param right away.

Use struct

Instead of separating _targets and _data into 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

File % Lines % Statements % Branches % Funcs
contracts/multicall/GuardedMulticaller2.sol 97.22% (35/36) 97.78% (44/45) 100.00% (20/20) 85.71% (6/7)
openzeppelin-code[bot] commented 2 months ago

GMS-2051 Multi-caller v2

Generated at commit: 2af8c046f91c38202f640a49dfe80645df5d0c25

🚨 Report Summary

Severity Level Results
Contracts Critical
High
Medium
Low
Note
Total
2
0
0
11
27
40
Dependencies Critical
High
Medium
Low
Note
Total
0
0
0
0
0
0

For more details view the full report in OpenZeppelin Code Inspector

drinkcoffee commented 2 months ago

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.

hiep-immutable commented 2 months ago

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.

@drinkcoffee I have updated our threat model with the following steps if such scenario happens: