Skip to content
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

refactor: Adding logic to check more L2 chains fo guard CM #122

Merged
merged 15 commits into from
May 13, 2024
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 2 additions & 0 deletions audits/README.md
Original file line number Diff line number Diff line change
Expand Up @@ -23,6 +23,8 @@ An internal audit with a focus on `Update for Community Multisig (CM)` is locate

An internal audit with a focus on `OptimismMesseger and WormholeMessenger` is located in this folder: [internal audit 9](https://github.com/valory-xyz/autonolas-governance/blob/main/audits/internal9).

An internal audit with a focus on `Guard for Community Multisig (CM) (modular version)` is located in this folder: [internal audit 10](https://github.com/valory-xyz/autonolas-governance/blob/main/audits/internal10).

### External audit
Following the initial contracts [audit report](https://github.com/valory-xyz/autonolas-governance/blob/main/audits/Valory%20Review%20Final.pdf),
the recommendations are addressed here: [feedback](https://github.com/valory-xyz/autonolas-governance/blob/main/audits/Addressing%20Initial%20ApeWorX%20Recommentations.pdf).
Expand Down
75 changes: 75 additions & 0 deletions audits/internal10/README.md
Original file line number Diff line number Diff line change
@@ -0,0 +1,75 @@
# autonolas-governance-audit
The review has been performed based on the contract code in the following repository:<br>
`https://github.com/valory-xyz/autonolas-governance` <br>
commit: `bde297d7dbb601fbe796f56fe03e917d60282cd0` or `tag: v1.1.11-pre-internal-audit` <br>

Update: 06-03-2024 <br>

## Objectives
The audit focused on Guard contract for community mutisig (modular version). <BR>

### Flatten version
Flatten version of contracts. [contracts](https://github.com/valory-xyz/autonolas-governance/blob/main/audits/internal10/analysis/contracts)

### Coverage
Hardhat coverage has been performed before the audit and can be found here:
```sh
--------------------------------------|----------|----------|----------|----------|----------------|
File | % Stmts | % Branch | % Funcs | % Lines |Uncovered Lines |
--------------------------------------|----------|----------|----------|----------|----------------|
contracts/multisigs/ | 98.18 | 96.15 | 100 | 99 | |
GuardCM.sol | 98.11 | 96.05 | 100 | 98.95 | 223 |
VerifyData.sol | 100 | 100 | 100 | 100 | |
contracts/multisigs/bridge_verifier/ | 49.12 | 47.37 | 50 | 49.44 | |
ProcessBridgedDataArbitrum.sol | 0 | 0 | 0 | 0 |... 55,56,60,64 |
ProcessBridgedDataGnosis.sol | 100 | 100 | 100 | 100 | |
ProcessBridgedDataOptimism.sol | 0 | 0 | 0 | 0 |... 75,76,80,83 |
ProcessBridgedDataPolygon.sol | 100 | 100 | 100 | 100 | |
ProcessBridgedDataWormhole.sol | 0 | 0 | 0 | 0 |... 67,72,73,77 |
VerifyBridgedData.sol | 100 | 100 | 100 | 100 | |
--------------------------------------|----------|----------|----------|----------|----------------|
```
Please, pay attention. More tests are needed and magic offsets (like MIN_ARBITRUM_PAYLOAD_LENGTH) can only be checked during testing
[x] fixed

### Storage timelock
Using sol2uml tools: https://github.com/naddison36/sol2uml <br>
```bash
sol2uml storage . -f png -c GuardCM -o .
Generated png file GuardCM.png
```
Storage: [GuardCM](https://github.com/valory-xyz/autonolas-governance/blob/main/audits/internal10/analysis/GuardCM.png)

### Security issues
Details in [slither_full](https://github.com/valory-xyz/autonolas-governance/blob/main/audits/internal10/analysis/slither_full.txt) <br>
All is false positive, discussed https://github.com/pessimistic-io/slitherin/blob/master/docs/arbitrary_call.md

Minor issue: <br>
- Cached bytes4(data)
```
Dubious typecast in VerifyData._verifyData(address,bytes,uint256) (ProcessBridgedDataArbitrum-flatten.sol#25-38):
bytes => bytes4 casting occurs in targetSelectorChainId |= uint256(uint32(bytes4(data))) << 160 (ProcessBridgedDataArbitrum-flatten.sol#30)
bytes => bytes4 casting occurs in revert NotAuthorized(address,bytes4,uint256)(target,bytes4(data),chainId) (ProcessBridgedDataArbitrum-flatten.sol#36)
```
[x] fixed

- Checking for verifierL2s is contract in set function.
```
function setBridgeMediatorL1BridgeParams(
if (verifierL2s[i].code.length == 0) {
revert AddressEmptyCode(verifierL2s[i]);
}
because if verifierL2s[i] is EOA
bridgeParams.verifierL2.delegatecall - always success,
ref: https://github.com/OpenZeppelin/openzeppelin-contracts/blob/master/contracts/utils/Address.sol#L124
```
[x] fixed

- Needing fix revert message
```
Code correct. Ref: https://github.com/OpenZeppelin/openzeppelin-contracts/blob/master/contracts/utils/Address.sol#L146
revert("Function call reverted");
replace to error-style message
revert FailedInnerCall();
```
[x] fixed
Binary file added audits/internal10/analysis/GuardCM.png
Loading
Sorry, something went wrong. Reload?
Sorry, we cannot display this file.
Sorry, this file is invalid so it cannot be displayed.
Loading
Loading