We reviewed the protofire/safe-contracts repository at the 17d9fd5491f7e63543f51082e733220adb17a832 commit.
In scope were the following contracts:
contracts
├── GnosisSafeZk.sol
└── GnosisSafeL2Zk.sol
However, the code under review is a minor modification to the established GnosisSafe and GnosisSafeL2 contracts. We did not review the code in audit-level detail and we didn't audit the Gnosis Safe v1.3.0 code itself, rather we focussed on identifying edge cases or potential unexpected behavior that might arise when deploying the contract to the new zkSync Era Layer 2 blockchain. Throughout the assessment we relied on the provided documentation about known differences between zkSync Era zkEVM and a canonical Ethereum EVM.
The GnosisSafe contract is an N-of-M multisignature account with several extensible features, introduced by "modules". It allows (N of) the owners to:
The zkSync Era Layer 2 blockchain is designed to provide an EVM-compatible (but not equivalent) environment so that Layer 1 solidity contracts can be run on Layer 2 with a few minor differences.
The GnosisSafeZk contract incorporates a minor change to address one of the differences. Specifically the send instruction in the handlePayment function has been changed to a call instruction instead. The GnosisSafeL2Zk contract is just an extension of GnosisSafeZk that adds two event emissions on top of GnosisSafeZk execTransaction and execTransactionFromModule functions.
The goal of this review was to assess whether this adapted contract can be expected to have the same behavior within the zkSync ecosystem that the GnosisSafe contract has on the Ethereum mainnet.
Any address is able to invoke a transaction on behalf of the GnosisSafeZk contract, provided the transaction has been authorized by enough owners and the relayer accepts the corresponding gas refund parameters. However, if no refundRecipient is specified, the tx.origin address will receive the refund.
This makes sense on the EVM but on the zkEVM there is an important subtlety. If the initiator is an EOA (the default account), then it is set as the tx.origin value, so it will correctly receive the refund. However, if the initiator is another smart contract account, the bootloader will be set as tx.origin and it will incorrectly receive the refund.
To avoid loss of funds, consider enforcing that the refundRecipient is specified whenever tx.origin is the bootloader address.
Update: Acknowledged, not resolved. The Matter Labs team stated:
We agree that this complicates the usage of custom accounts with multisig wallet. However, the cost of supporting different codebases is also very high and the issue affects only custom zkSync functionality.
The zkSync Era documentation strongly recommends using the latest 0.8.x version of Solidity. However, the code under review allows all versions from 0.7.0 and the project is configured to use version 0.7.6. To comply with best practices, consider compiling with the latest Solidity version.
Update: Acknowledged, not resolved. The Matter Labs team stated:
While it is always better to use the latest version of the compiler, there are no known issues that affect the codebase.
Here are some suggestions for improving the clarity and accuracy of the documentation:
creationCode is not supported should instead say that it translates to the bytecode hash..send or .transfer replacement should check the success flag after the call.SystemContractsCaller explanation contains a broken link to the contract implementation.Consider updating the documentation accordingly.
Update: Partially resolved in pull request #591 at commits 6d77af8 and d446f94, with second suggestion not being addressed.
The existing codebase uses hardcoded constants to determine the transaction execution overhead and the token transfer overhead, which contradicts the recommended best practice. Consider whether these constants are still valid under the zkSync Era fee model, and whether it would be appropriate to remove the constants altogether.
Update: Acknowledged, not resolved. The Matter Labs team acknolwedged this issue.
During the audit we incidentally identified a minor undesirable incentive. As noted in the documentation, only state diffs are stored on Ethereum mainnet. The transaction cost for a storage change is only charged once to the first user in the block that made the change. This creates an incentive for users to backrun other transactions that modify the same storage slots, since subsequent changes will not be charged.
The incentive is capped at the cost of all storage changes in the transaction, and is partially mitigated by the fact that currently all transactions are sent directly to the sequencer. Consider whether it's worth socializing the cost to every user interacting with the same storage slot.
Update: Acknowledged, not resolved. The Matter Labs team stated:
We will take this into account with updating the fee model mechanism.
When refunding the gas costs, the returned data variable is unused.
Consider leaving it undeclared.
Update: Resolved. The Matter Labs team stated:
Fixed with using original GnosisSafe/GnosisSafeL2 contract codes.
Although not required for zkSync compatibility, we identified several recommendations to improve the original Gnosis functionality.
The code uses a hardcoded sentinel address of 0x1 to define the owner linked list and the module linked list. This is intended to be a placeholder address, but it corresponds to the Ecrecover system contract on the zkEVM (and the ecrecover precompile on the EVM).
This introduces the possibility that the Ecrecover system contract could incorrectly invoke the approveHash function. This will not be an issue in practice, but in the interest of predictability, consider choosing a pseudorandom address that is not being used by a system contract.
The GnosisSafeZk contract uses different styles of docstrings simultaneously. For example, compare the docstrings of the approveHash function with the requiredTxGas function. Consider unifying the style.
The start parameter of the getModulesPaginated function is supposed to be at the start of the page. However, the first returned value is the module immediately after the start module. This means that neither the start module nor the returned next module are contained in the page. If the whole list is enumerated by passing the next module as the start of the next page, subsequent pages will have a missing module between them.
Consider updating the code to return the start module as the first element on the page.
The GnosisSafeL2Zk contract emits SafeMultiSigTransaction and SafeModuleTransaction events whether or not the actual operation succeeds, so they need to be correlated with the success or failure events for accurate tracking. Consider including the transaction result in the event message.
Moreover, these events (as well as several events in the GnosisSafeZk contract) do not index their parameters. Our recommendation is to consider indexing event parameters to improve the ability of off-chain services to search and filter for specific events.
Some parts of the codebase do not have docstrings. For instance:
Some parts of the code have misleading docstrings. For instance:
GnosisSafeZk.sol says funds are being sent to the tx.origin, while that's not necessarily true.GuardManager.sol incorrectly describes the FallbackManager contract.Consider adding docstrings where needed and fixing incorrect ones to improve the overall readability of the codebase. When writing docstrings, consider following the Ethereum Natural Specification Format (NatSpec).
require Statement With Multiple ConditionsWithin GnosisSafeZk.sol there is a require statement on line 303 that requires multiple conditions to be satisfied.
To simplify the codebase and raise the most helpful error messages for failing require statements, consider having a single require statement per condition.
Singleton Contract Should Be First in the Inheritance ChainThe comment on the Singleton contract explicitly states that it should be positioned in the inheritance chain to align the singleton variable. However, it is positioned as second instead. In this case, the singleton variable is still positioned correctly, but this cannot be ascertained by local reasoning. To increase robustness, consider moving it to the top of the inheritance list.
The GnosisSafeZk contract uses 0x20c13b0b as the EIP-1271 magic constant, corresponding to the old isValidSignature function interface. However, the latest version of the EIP uses a new interface and a corresponding new constant. Consider complying with the latest definition of the EIP.
signedMessages MappingThe signedMessages mapping and SignMsg event are not used in the deployed code. They are referenced in the SignMessageLib contract (which redefines the event) and appear to offer similar functionality to the approvedHashes mapping.
Consider whether it should be updated in the approveHash function (to update the number of people who accept the transaction), or in execTransaction after it's been executed, or if it should be removed entirely.
The add(result, 0x20) operation in the getStorageAt function of StorageAccessible contract can be moved outside the loop. This would also be an opportunity to introduce a comment explaining why the 0x20 offset is used (to account for the length field), which would improve the readability of the code.
This assessment was conducted over the course of a week, and we uncovered a handful of medium and lower-severity issues as well as a number of general recommendations related to Safe functionality.