Update Contract #8
Open
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Merge main into checkSignatures branch
…-fix Formal verification: Fix unresolved setup call from the harness constructor
Fix typo in buildSignatureBytes comment
Add 1.5.0 changelog
Feature: migration to `hardhat-toolbox`, ethers v6
Formal verification: Remove rules overlapping with rules in OwnerReach.spec
…ization Use byte instead of and in the SignatureDecoderContract
Chore: Remove ewc and volta from the hardhat config
Formal verification: Add rules that check only modules can execute module transactions
Fix: Solidity linter errors
This PR does a once-over on memory Safety for the 1.5.0 contracts. In particular, there were a couple of missing `memory-safe` tags for some assembly blocks which were preventing the IR assembler from working correctly. Additionally, since the `MultiSend*` contracts changed anyway in 1.5.0, I took this opportunity to change the assembly to be memory-safe so that we can tag it. Note that it only adds more code in the revert case, so it should not have a negative impact for most use-cases. Furthermore, I added a comment explaining why we did not make the `SafeProxy` contract memory-safe (that is one intentional). Lastly, I noticed that there were some `eq(..., 0)` assembly calls which can be written as `iszero(...)` to save some gas and code. Again, it was an opportunistic change as the affected contracts have changed anyway and will be re-audited. --------- Co-authored-by: Shebin John <[email protected]>
It is now ~~1.4.1-2~~ 1.4.1-3 with the migration contracts.
Matter Labs recently renamed `era-test-node` to `zksync-anvil`. This
causes the official `hardhat-zksync-node` package to stop working.
It is possible to patch the NPM package so that it works with `sed`:
```sh
find node_modules/@matterlabs/hardhat-zksync-node/ -type f -exec sed -i "s/era[_-]test[_-]node/anvil-zksync/g" '{}' ';'
```
However, the tests seem to be quite flaky (ran on my machine once, but
the `anvil` node seems to crash quite often).
This pull request includes a small but significant change to the `package.json` file to enhance the testing process. Enhancements to testing: * [`package.json`](diffhunk://#diff-7ae45ad102eab3b6d7e7896acd08c427a9b25b346470d7bc6507b6481575d519L22-R22): Modified the `test` script to include running `test:zk` to ensure zkSync tests are executed along with other tests.
The migration tests are defining `describe` blocks asyncronously within
an `it` block. AFAIU, this is a no-no and causes ugly test formatting.
This PR adjusts the test definitions so that they belong to the correct
parent `describe` block (one per migration version).
<details><summary>Test formatting <strong>before</strong>:</summary>
```
addOwner
✔ should add owner and change threshold
enableModule
✔ should enabled module and be able to use it
multiSend
✔ execute multisend via delegatecall
fallbackHandler
✔ should be correctly set
execTransaction
✔ should be able to transfer ETH (48ms)
addOwner
✔ should add owner and change threshold
enableModule
✔ should enabled module and be able to use it
multiSend
✔ execute multisend via delegatecall
fallbackHandler
✔ should be correctly set
execTransaction
✔ should be able to transfer ETH (48ms)
addOwner
✔ should add owner and change threshold
enableModule
✔ should enabled module and be able to use it
multiSend
✔ execute multisend via delegatecall
fallbackHandler
✔ should be correctly set
execTransaction
✔ should be able to transfer ETH (45ms)
addOwner
✔ should add owner and change threshold
enableModule
✔ should enabled module and be able to use it
multiSend
✔ execute multisend via delegatecall
fallbackHandler
✔ should be correctly set
execTransaction
✔ should be able to transfer ETH (49ms)
addOwner
✔ should add owner and change threshold
enableModule
✔ should enabled module and be able to use it
multiSend
✔ execute multisend via delegatecall
fallbackHandler
✔ should be correctly set
execTransaction
✔ should be able to transfer ETH (52ms)
addOwner
✔ should add owner and change threshold
enableModule
✔ should enabled module and be able to use it
multiSend
✔ execute multisend via delegatecall
fallbackHandler
✔ should be correctly set
```
</details>
<details><summary>Test formatting <strong>after</strong>:</summary>
```
Upgrade from Safe 1.1.1
execTransaction
✔ should be able to transfer ETH (50ms)
addOwner
✔ should add owner and change threshold
enableModule
✔ should enabled module and be able to use it
multiSend
✔ execute multisend via delegatecall
fallbackHandler
✔ should be correctly set
Upgrade from Safe 1.2.0
execTransaction
✔ should be able to transfer ETH (45ms)
addOwner
✔ should add owner and change threshold
enableModule
✔ should enabled module and be able to use it
multiSend
✔ execute multisend via delegatecall
fallbackHandler
✔ should be correctly set
Upgrade from Safe 1.3.0
execTransaction
✔ should be able to transfer ETH (52ms)
addOwner
✔ should add owner and change threshold
enableModule
✔ should enabled module and be able to use it
multiSend
✔ execute multisend via delegatecall
fallbackHandler
✔ should be correctly set
Upgrade from Safe 1.3.0 L2
execTransaction
✔ should be able to transfer ETH (50ms)
addOwner
✔ should add owner and change threshold
enableModule
✔ should enabled module and be able to use it
multiSend
✔ execute multisend via delegatecall
fallbackHandler
✔ should be correctly set
Upgrade from Safe 1.4.1
execTransaction
✔ should be able to transfer ETH (52ms)
addOwner
✔ should add owner and change threshold
enableModule
✔ should enabled module and be able to use it
multiSend
✔ execute multisend via delegatecall
fallbackHandler
✔ should be correctly set
Upgrade from Safe 1.4.1 L2
execTransaction
✔ should be able to transfer ETH (51ms)
addOwner
✔ should add owner and change threshold
enableModule
✔ should enabled module and be able to use it
multiSend
✔ execute multisend via delegatecall
fallbackHandler
✔ should be correctly set
```
</details>
This commit disables support for pre-approved signatures from `msg.sender` for `isValidSignature` calls. Note that we remove the new 1.5.0-only `checkSignatures` interface in favour of one with `executor` explicitly provided to avoid issues for contracts that make use of this function.
This PR fixes the shift amounts in the `SafeProxy` implementation. It also slightly optimizes how we do the masking by 2 instructions (removes the need to `PUSH 0` and `SHR`). I added some additional tests to make sure the masking works as expected and a comment explaining why we only mask for handling the `masterCopy()` call. --------- Co-authored-by: Mikhail <[email protected]>
Co-authored-by: Nicholas Rodrigues Lordello <[email protected]>
…sh` (#872) This commit adds a detailed warning in the `Safe.sol` contract regarding potential dirty bits in assembly code for types smaller than 256 bits. It emphasizes the importance of considering this for future changes while explaining the rationale behind using assembly for memory efficiency. No functional changes were made to the contract logic. This update aims to improve code clarity and maintainability. Originally reported by @jhoenicke
This PR focuses on correcting typos and improving clarity in documentation files. Signed-off-by: chloefeal <[email protected]>
Corrected `considely` to `concisely` Corrected `hhttps` to `https`
This PR focuses on correcting typos and improving clarity in comments. Thank you very much. Signed-off-by: oliveredget <[email protected]>
Hello, I fix some typos to improve clarity and correctness. Thank you very much. Signed-off-by: calciumbe <[email protected]>
This pull request contains changes to improve clarity and correctness. Description correction: typo fix. Thank you very much. Signed-off-by: dxsullivan <[email protected]>
…889) This pull request includes a change which allows 1 SLOAD to be saved by emitting an existing memory variable instead of reading from storage. * [`contracts/base/OwnerManager.sol`](diffhunk://#diff-795fb06764b4c2d991707584a31509badf0b036c9401bfbcb82d6bc9fdebab82L113-R113): Modified the `emit` statement in the `setThreshold` function to use the `_threshold` parameter instead of `threshold`, ensuring the value from memory is emitted.
…ges for encodeData and payload lengths (#873) This pull request includes modifying the `SignatureVerifierMuxer` contract to correct the byte offsets for encoding and payload data. Changes to byte offsets: * [`contracts/handler/extensible/SignatureVerifierMuxer.sol`](diffhunk://#diff-62f21ce8850527f34ef2acdacd96d4a2a1150e3e2a7e16457e82236bbd4259d2L124-R127): Updated the byte offset ranges for `encodeData length`, `encodeData`, and `payload length` to correct values.
…backManager.sol` (#879) Updated the documentation for the fallback handler in both `Safe.sol` and `IFallbackManager.sol` to improve clarity and highlight security risks associated with setting the fallback handler. Added a warning about the potential for bypassing access control mechanisms when using untrusted addresses.
…ion details (#885) This PR adds a detailed comment in the TokenCallbackHandler contract, clarifying the requirement for accounts to register the implementer via the ERC-1820 interface registry to receive ERC777 tokens. This update aims to improve clarity and understanding of the token reception process. No functional changes were made to the contract logic.
…6` (#886) This pull request includes updates to comments in the Solidity smart contracts to clarify the usage of the `keccak256` function. The changes ensure that the comments accurately reflect the encoding process used in the contracts. Changes to comments for clarity: * [`contracts/common/SecuredTokenTransfer.sol`](diffhunk://#diff-7a34930a339acfe3b45e163bee3e08df2132c01826e6e03771827a4181c6f567L19-R19): Updated the comment to specify that `0xa9059cbb` is the `bytes4` encoding of `keccak256("transfer(address,uint256)")`. * [`contracts/proxies/SafeProxy.sol`](diffhunk://#diff-754f853ea53666aa85b2d27bbcc0623b7cfd83449e0b949eed39dde8f01ba1cdL41-R41): Updated the comment to specify that `0xa619486e` is the `bytes4` encoding of `keccak256("masterCopy()")`. Co-authored-by: Mikhail <[email protected]>
…ved in the normal path (#888) This pull request includes a small but important change to the `removeOwner` function in the `OwnerManager` contract. The change optimizes the decrement operation for the `ownerCount` variable. The previous code reads from storage twice with the `ownerCount` variable. By doing pre-decrement (which is cheaper than post-decrement), we can save on 1 SLOAD. Optimization: * [`contracts/base/OwnerManager.sol`](diffhunk://#diff-795fb06764b4c2d991707584a31509badf0b036c9401bfbcb82d6bc9fdebab82L74-L80): Modified the `removeOwner` function to use pre-decrement for `ownerCount` to improve efficiency and ensure the threshold check is performed correctly.
…and storage access optimization (#890) This pull request includes an important update to the `ERC165Handler` contract to improve the handling of interface support and streamline the code. The most significant changes are as follows: ### The following optimizes the logic and minimizes storage accesses: * [`contracts/handler/extensible/ERC165Handler.sol`](diffhunk://#diff-aa0838f20fd3f37b00dc661645b4641500e68762b9b624addb99465fcc65a3e0L38-R48): Refactored the logic for checking and updating the support status of interfaces by introducing a local `mapping` reference. This change simplifies the code and ensures that the interface support status is updated correctly.
…ss optimization (#891) This pull request includes a refactor to the `_setSafeMethod` function in the `ExtensibleBase` contract to improve memory access. The following optimizes the logic and minimizes storage accesses: * [`contracts/handler/extensible/ExtensibleBase.sol`](diffhunk://#diff-e395e05c7e2951461c2e599137367a6a3304e849f3c522903d74f1a50f23b577L49-R57): Modified the `_setSafeMethod` function to use a local variable `safeMethod` for accessing the `safeMethods` mapping, which simplifies and clarifies the code.
This pull request includes a critical update to the `SafeProxyFactory` contract in the `contracts/proxies/SafeProxyFactory.sol` file. The change improves the gas cost of the assembly code by replacing the `eq` function with the `iszero` function. Key change: * [`contracts/proxies/SafeProxyFactory.sol`](diffhunk://#diff-53aac98a01e16743466b00bdcb233208f95e1113cc90b95181187d9862b4ad6eL43-R43): Replaced the `eq` function with the `iszero` function in the assembly block to improve code readability and reliability.
…`: save gas via short-circuit evaluation (#893) This pull request includes a change to the `ExtensibleFallbackHandler` contract in the `contracts/handler/ExtensibleFallbackHandler.sol` file. The change modifies the `_supportsInterface` function to reorder the interface checks for `ERC721TokenReceiver`, `ERC1155TokenReceiver`, and `IFallbackHandler`. This helps in taking advantage of the short-circuit evaluation. Changes to interface support order: * [`contracts/handler/ExtensibleFallbackHandler.sol`](diffhunk://#diff-aa345618c4d3f173b09e211d0bd0eec0747177aab345bf8b9f5bbc874a765fe3R21-R26): Reordered the interface checks in the `_supportsInterface` function to place `ERC721TokenReceiver` and `ERC1155TokenReceiver` before `IFallbackHandler`.
This pull request includes several optimizations to the `OwnerManager` and `ERC165Handler` contracts by reducing the number of times array lengths are accessed within loops. These changes aim to improve the efficiency of the code by storing the array lengths in variables before the loops. If not cached, the solidity compiler will always read the length of the array during each iteration. That is, if it is a storage array, this is an extra sload operation (100 additional extra gas for each iteration except for the first) and if it is a memory array, this is an extra mload operation (3 additional gas for each iteration except for the first). Optimizations in `OwnerManager` contract: * [`contracts/base/OwnerManager.sol`](diffhunk://#diff-795fb06764b4c2d991707584a31509badf0b036c9401bfbcb82d6bc9fdebab82L38-R39): Stored `_owners.length` in a variable `ownersLength` before the loop to avoid repeatedly accessing the array length. * [`contracts/base/OwnerManager.sol`](diffhunk://#diff-795fb06764b4c2d991707584a31509badf0b036c9401bfbcb82d6bc9fdebab82L49-R50): Updated `ownerCount` to use the `ownersLength` variable instead of accessing `_owners.length` again. Optimizations in `ERC165Handler` contract: * [`contracts/handler/extensible/ERC165Handler.sol`](diffhunk://#diff-aa0838f20fd3f37b00dc661645b4641500e68762b9b624addb99465fcc65a3e0L56-R57): Stored `handlerWithSelectors.length` in a variable `len` before the loop in `addSupportedInterfaceBatch` to avoid repeatedly accessing the array length. * [`contracts/handler/extensible/ERC165Handler.sol`](diffhunk://#diff-aa0838f20fd3f37b00dc661645b4641500e68762b9b624addb99465fcc65a3e0L78-R80): Stored `selectors.length` in a variable `len` before the loop in `removeSupportedInterfaceBatch` to avoid repeatedly accessing the array length.
…d `performCreate2` (#887) Commutativity makes the two additions equivalent but Certora recommend the fix below for readability and to follow the standard given that: - `deploymentData` gives a pointer to the start of the array (length position). - Adding `0x20` skips the first 32 bytes (length field) to point directly to the start of the payload. Change: * [`contracts/libraries/CreateCall.sol`](diffhunk://#diff-d5d801f238d69f94974c4f4628197fcc2df478f608c18c5f691f73dfd552e36eL25-R25): Changed the order of parameters in the `create2` function call to correctly add the offset to `deploymentData`. Co-authored-by: Mikhail <[email protected]>
Added a comment in the Safe contract to clarify that the `s` value of ECDSA signatures is not enforced to be in the lower half of the curve. This note explains the implications of ECDSA malleability and reassures that existing mechanisms are in place to prevent duplicate signatures and replay attacks. No functional changes were made to the contract logic.
This pull request includes several changes to increment and decrement operations in various Solidity contract files. The primary goal is to decrease gas usage. Pre-increments and pre-decrements are cheaper. For a uint256 i variable, the following is true with the Optimizer enabled at 10k: Increment: - i += 1 is the most expensive form - i++ costs 6 gas less than i += 1 - ++i costs 5 gas less than i++ (11 gas less than i += 1) Decrement: - i -= 1 is the most expensive form - i-- costs 11 gas less than i -= 1 - --i costs 5 gas less than i-- (16 gas less than i -= 1) Changes to increment and decrement operations: * [`contracts/Safe.sol`](diffhunk://#diff-587b494ea631bb6b7adf4fc3e1a2e6a277a385ff16e1163b26e39de24e9483deL296-R296): Updated the for loop to use the prefix increment operator in the `Safe` contract. * [`contracts/base/ModuleManager.sol`](diffhunk://#diff-82762908b9416ddadffb149ee4d25f328078fc27f938d454d8a207aad1ec3839L215-R215): Changed the increment operation to use the prefix increment operator in the `ModuleManager` contract. * [`contracts/base/OwnerManager.sol`](diffhunk://#diff-795fb06764b4c2d991707584a31509badf0b036c9401bfbcb82d6bc9fdebab82L38-R38): Multiple updates to use prefix increment and decrement operators in the `OwnerManager` contract. [[1]](diffhunk://#diff-795fb06764b4c2d991707584a31509badf0b036c9401bfbcb82d6bc9fdebab82L38-R38) [[2]](diffhunk://#diff-795fb06764b4c2d991707584a31509badf0b036c9401bfbcb82d6bc9fdebab82L63-R63) [[3]](diffhunk://#diff-795fb06764b4c2d991707584a31509badf0b036c9401bfbcb82d6bc9fdebab82L80-R80) [[4]](diffhunk://#diff-795fb06764b4c2d991707584a31509badf0b036c9401bfbcb82d6bc9fdebab82L142-R142) * [`contracts/common/StorageAccessible.sol`](diffhunk://#diff-a7dd65d90b0567bb9ba14ecd4ff414529a934cd3752ccf309800fad93fba354eL19-R19): Modified the for loop to use the prefix increment operator in the `StorageAccessible` contract. * [`contracts/handler/extensible/ERC165Handler.sol`](diffhunk://#diff-aa0838f20fd3f37b00dc661645b4641500e68762b9b624addb99465fcc65a3e0L56-R56): Updated for loops to use the prefix increment operator in the `ERC165Handler` contract. [[1]](diffhunk://#diff-aa0838f20fd3f37b00dc661645b4641500e68762b9b624addb99465fcc65a3e0L56-R56) [[2]](diffhunk://#diff-aa0838f20fd3f37b00dc661645b4641500e68762b9b624addb99465fcc65a3e0L78-R78) * [`contracts/libraries/SafeToL2Migration.sol`](diffhunk://#diff-925588b812f729cc164d14a48e571ce813e2f0ae6f5c5420fc0382c767287fd0L188-R188): Changed the increment operation to use the prefix increment operator in the `SafeToL2Migration` contract.
) This pull request includes changes to the `contracts/handler/extensible` directory, specifically in the `MarshalLib.sol` and `SignatureVerifierMuxer.sol` files. The changes focus on improving the handling of data and selectors within assembly code blocks to decrease gas usage. Improvements to data handling and selector extraction: * [`contracts/handler/extensible/MarshalLib.sol`](diffhunk://#diff-7122c44132b6fc89cd7c9f3c48519c88aaf7308705a1170d307d72465eb9e1c9L41-R41): Modified the way the `handler` is extracted from `data` by using a bitwise AND operation to ensure proper extraction of the handler address. [[1]](diffhunk://#diff-7122c44132b6fc89cd7c9f3c48519c88aaf7308705a1170d307d72465eb9e1c9L41-R41) [[2]](diffhunk://#diff-7122c44132b6fc89cd7c9f3c48519c88aaf7308705a1170d307d72465eb9e1c9L59-R59) * [`contracts/handler/extensible/SignatureVerifierMuxer.sol`](diffhunk://#diff-62f21ce8850527f34ef2acdacd96d4a2a1150e3e2a7e16457e82236bbd4259d2L113-R113): Changed the extraction of `sigSelector` from `calldataload` to use a bitwise AND operation for more accurate and secure selector extraction.
…lication if possible (#895) This pull request includes optimizations to the gas usage in the `Safe.sol` and `StorageAccessible.sol` contracts by replacing multiplication operations with bitwise shift operations. While the `DIV / MUL` opcode uses 5 gas, the `SHR / SHL` opcode only uses 3 gas. Gas optimization changes: * [`contracts/Safe.sol`](diffhunk://#diff-587b494ea631bb6b7adf4fc3e1a2e6a277a385ff16e1163b26e39de24e9483deL168-R169): Replaced the multiplication operation `* 64` with the bitwise shift operation `<< 6` to reduce gas costs in the `gasleft` check. * [`contracts/common/StorageAccessible.sol`](diffhunk://#diff-a7dd65d90b0567bb9ba14ecd4ff414529a934cd3752ccf309800fad93fba354eL18-R18): Replaced the multiplication operation `* 32` with the bitwise shift operation `<< 5` to reduce gas costs when creating a new bytes array.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
No description provided.