-
Notifications
You must be signed in to change notification settings - Fork 360
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
feat: mutable storage ISMs #4577
base: audit-q3-2024
Are you sure you want to change the base?
Conversation
🦋 Changeset detectedLatest commit: e9f7731 The changes in this PR will be included in the next version bump. This PR includes changesets to release 9 packages
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
@@ -0,0 +1,63 @@ | |||
// SPDX-License-Identifier: MIT OR Apache-2.0 | |||
pragma solidity >=0.8.0; |
Check notice
Code scanning / Olympix Integrated Security
Using an unbounded pragma for Solidity version may be unsafe if future versions introduce breaking changes. For more information, visit: http://detectors.olympixdevsectools.com/article/web3-vulnerability/unbounded-pragma Low
|
||
abstract contract AbstractStorageMultisigIsm is AbstractMultisigIsm, Ownable { | ||
address[] public validators; | ||
uint8 public threshold; |
Check notice
Code scanning / Olympix Integrated Security
Some state variables are not being fuzzed in test functions, potentially leaving vulnerabilities unexplored. For more information, visit: http://detectors.olympixdevsectools.com/article/web3-vulnerability/unfuzzed-variables Low
|
||
constructor( | ||
address[] memory _validators, | ||
uint8 _threshold |
Check notice
Code scanning / Olympix Integrated Security
Parameters passed to a constructor that are not validated for correct values may lead to contract creation in an undesired state. For more information, visit: http://detectors.olympixdevsectools.com/article/web3-vulnerability/no-parameter-validation-in-constructor Low
AbstractMessageIdMultisigIsm, | ||
AbstractStorageMultisigIsm | ||
{ | ||
uint8 public constant moduleType = |
Check notice
Code scanning / Olympix Integrated Security
Some state variables are not being fuzzed in test functions, potentially leaving vulnerabilities unexplored. For more information, visit: http://detectors.olympixdevsectools.com/article/web3-vulnerability/unfuzzed-variables Low
uint8(IInterchainSecurityModule.Types.MESSAGE_ID_MULTISIG); | ||
|
||
constructor( | ||
address[] memory _validators, |
Check notice
Code scanning / Olympix Integrated Security
Parameters passed to a constructor that are not validated for correct values may lead to contract creation in an undesired state. For more information, visit: http://detectors.olympixdevsectools.com/article/web3-vulnerability/no-parameter-validation-in-constructor Low
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## audit-q3-2024 #4577 +/- ##
=================================================
+ Coverage 73.74% 73.92% +0.18%
=================================================
Files 100 102 +2
Lines 1436 1469 +33
Branches 187 189 +2
=================================================
+ Hits 1059 1086 +27
- Misses 356 362 +6
Partials 21 21
|
@@ -0,0 +1,9 @@ | |||
// SPDX-License-Identifier: MIT OR Apache-2.0 | |||
pragma solidity >=0.8.0; |
Check notice
Code scanning / Olympix Integrated Security
Using an unbounded pragma for Solidity version may be unsafe if future versions introduce breaking changes. For more information, visit: http://detectors.olympixdevsectools.com/article/web3-vulnerability/unbounded-pragma Low
event ValidatorsAndThresholdSet(address[] validators, uint8 threshold); | ||
|
||
constructor( | ||
address[] memory _validators, |
Check notice
Code scanning / Olympix Integrated Security
Parameters passed to a constructor that are not validated for correct values may lead to contract creation in an undesired state. For more information, visit: http://detectors.olympixdevsectools.com/article/web3-vulnerability/no-parameter-validation-in-constructor Low
|
||
constructor( | ||
address[] memory _validators, | ||
uint8 _threshold |
Check notice
Code scanning / Olympix Integrated Security
Parameters passed to a constructor that are not validated for correct values may lead to contract creation in an undesired state. For more information, visit: http://detectors.olympixdevsectools.com/article/web3-vulnerability/no-parameter-validation-in-constructor Low
|
||
constructor( | ||
address[] memory _validators, | ||
uint8 _threshold |
Check notice
Code scanning / Olympix Integrated Security
Parameters passed to a constructor that are not validated for correct values may lead to contract creation in an undesired state. For more information, visit: http://detectors.olympixdevsectools.com/article/web3-vulnerability/no-parameter-validation-in-constructor Low
} | ||
|
||
contract StorageMerkleRootMultisigIsmFactory is StorageMultisigIsmFactory { | ||
address internal immutable _implementation; |
Check notice
Code scanning / Olympix Integrated Security
Some state variables are not being fuzzed in test functions, potentially leaving vulnerabilities unexplored. For more information, visit: http://detectors.olympixdevsectools.com/article/web3-vulnerability/unfuzzed-variables Low
} | ||
|
||
contract StorageMessageIdMultisigIsmFactory is StorageMultisigIsmFactory { | ||
address internal immutable _implementation; |
Check notice
Code scanning / Olympix Integrated Security
Some state variables are not being fuzzed in test functions, potentially leaving vulnerabilities unexplored. For more information, visit: http://detectors.olympixdevsectools.com/article/web3-vulnerability/unfuzzed-variables Low
logger, | ||
config.threshold, | ||
); | ||
const deployStatic = (factory: StaticThresholdAddressSetFactory) => |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
will need to dupe this change to EvmIsmModule
too
protected async deployMultisigIsm({ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
brutal, I thought these were kinda using the same codepaths 😱
'Enter validator addresses (comma separated list) for multisig ISM:', | ||
}); | ||
const validators = validatorsInput.split(',').map((v) => v.trim()); | ||
const threshold = parseInt( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
maybe also check the given threshold >=1 and <= validators.length
should we also recommend a value?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
trying to leave input validation to the zod schema
can make this change there I suppose though 0/n as a noop ISM could be useful
} | ||
|
||
function initialize( | ||
address _owner, |
Check notice
Code scanning / Olympix Integrated Security
Shadowing state variables may lead to unintended behavior. For more information, visit: http://detectors.olympixdevsectools.com/article/web3-vulnerability/shadowing-state Low
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I mostly did a cursory review, so would like @aroralanuk to take a deeper look for the approval
import {TypeCasts} from "../../contracts/libs/TypeCasts.sol"; | ||
import {MerkleTreeHook} from "../../contracts/hooks/MerkleTreeHook.sol"; | ||
import {TestMerkleTreeHook} from "../../contracts/test/TestMerkleTreeHook.sol"; | ||
import {TestPostDispatchHook} from "../../contracts/test/TestPostDispatchHook.sol"; | ||
import {Message} from "../../contracts/libs/Message.sol"; | ||
import {ThresholdTestUtils} from "./IsmTestUtils.sol"; | ||
import {StorageMessageIdMultisigIsm, StorageMerkleRootMultisigIsm, StorageMessageIdMultisigIsmFactory, StorageMerkleRootMultisigIsmFactory, AbstractStorageMultisigIsm} from "../../contracts/isms/multisig/StorageMultisigIsm.sol"; | ||
|
||
uint8 constant MAX_VALIDATORS = 20; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
A comment on why this value is 20 might be nice
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this was pretty arbitrary tbh
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
bumped it from 10 to 20
@@ -1,41 +0,0 @@ | |||
import { expect } from 'chai'; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why is this getting removed?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
we no longer use this config schema
ce0e9a7
to
b51f1d3
Compare
### Description - Implements storage-based aggregation ISM ### Drive-by changes - Moves `PackageVersioned` imports from static to abstract aggregation ism ### Backward compatibility Yes ### Testing Unit Tests
@@ -0,0 +1,83 @@ | |||
// SPDX-License-Identifier: MIT OR Apache-2.0 | |||
pragma solidity >=0.8.0; |
Check notice
Code scanning / Olympix Integrated Security
Using an unbounded pragma for Solidity version may be unsafe if future versions introduce breaking changes. For more information, visit: http://detectors.olympixdevsectools.com/article/web3-vulnerability/unbounded-pragma Low
import {OwnableUpgradeable} from "@openzeppelin/contracts-upgradeable/access/OwnableUpgradeable.sol"; | ||
|
||
contract StorageAggregationIsm is AbstractAggregationIsm, OwnableUpgradeable { | ||
address[] public modules; |
Check notice
Code scanning / Olympix Integrated Security
Some state variables are not being fuzzed in test functions, potentially leaving vulnerabilities unexplored. For more information, visit: http://detectors.olympixdevsectools.com/article/web3-vulnerability/unfuzzed-variables Low
event ModulesAndThresholdSet(address[] modules, uint8 threshold); | ||
|
||
constructor( | ||
address[] memory _modules, |
Check notice
Code scanning / Olympix Integrated Security
Parameters passed to a constructor that are not validated for correct values may lead to contract creation in an undesired state. For more information, visit: http://detectors.olympixdevsectools.com/article/web3-vulnerability/no-parameter-validation-in-constructor Low
|
||
constructor( | ||
address[] memory _modules, | ||
uint8 _threshold |
Check notice
Code scanning / Olympix Integrated Security
Parameters passed to a constructor that are not validated for correct values may lead to contract creation in an undesired state. For more information, visit: http://detectors.olympixdevsectools.com/article/web3-vulnerability/no-parameter-validation-in-constructor Low
} | ||
|
||
function initialize( | ||
address _owner, |
Check notice
Code scanning / Olympix Integrated Security
Shadowing state variables may lead to unintended behavior. For more information, visit: http://detectors.olympixdevsectools.com/article/web3-vulnerability/shadowing-state Low
IThresholdAddressFactory, | ||
PackageVersioned | ||
{ | ||
address public immutable implementation; |
Check notice
Code scanning / Olympix Integrated Security
Some state variables are not being fuzzed in test functions, potentially leaving vulnerabilities unexplored. For more information, visit: http://detectors.olympixdevsectools.com/article/web3-vulnerability/unfuzzed-variables Low
Description
Some chains like zkSync do not support eip1167 (minimal/meta) proxies. This PR adds an alternative storage based multisig and aggregation ISM for use on these chains.
Drive-by changes
Simplify CLI multisig interactive config builder. Remove stale multisig config.
Related issues
None
Backward compatibility
Yes, relayer already supports this module type
Testing
Contract unit tests
Manual CLI tests