Conversation
abresas
force-pushed
the
audit-fixes
branch
5 times, most recently
from
d316d23 to
311e428
Compare
Bridge Audit Report finding (1) recommended inheriting AccessControlUpgradeable for forward-compatibility. In case in the future we update the openzeppelin contract versions, in order to be forward compatible we also: * call AccessControl_init even if it is just an empty function * inherit Initializer from the openzeppelin upgradeable contracts even though its just a re-export of the non-upgradeable version.
… one function This was (1) finding of the Informational Findings of the Bridge Audit Report. Parameter names were kept in order to produce the least amount of diff. A new test suite was added with a harness contract that allows directly calling the new internal function. Additionally, fuzz tests were implemented to cover more cases.
The Bridge Audit Report finding (2) of informational findings recommended changing the behabiour of checkInLimits so that splitting an action does not lead to a different result. This was achieved by setting the remaining capacity equal to the excess amount. As a result of this, larger that limit amounts where allowed to be posted, so that again, splitting a large amount into many small amounts does not make a difference. Unit tests and fuzz tests were added as well.
Per Bridge Audit Report informational finding #3.
There was a problem hiding this comment.
Pull request overview
This PR addresses audit findings by fixing critical issues in the Bridge contract's limit checking logic and updating to upgradeable OpenZeppelin contracts. The changes prevent a vulnerability where splitting deposits across period resets could reduce limit usage, and ensure proper initialization of upgradeable contracts.
Changes:
- Fixed daily limit logic to properly handle deposits spanning reset periods by tracking excess separately
- Migrated from standard OpenZeppelin contracts to upgradeable versions with proper initialization
- Added comprehensive test coverage for validator set updates and limit checking edge cases
Reviewed changes
Copilot reviewed 7 out of 8 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| solidity-sdk/lib/openzeppelin-contracts-upgradeable | Added OpenZeppelin upgradeable contracts submodule |
| protocol/test/Bridge.t.sol | Added tests for deposit limit boundary conditions and validator set management |
| protocol/test/Bridge.fork.sol | Reformatted code for consistency |
| protocol/src/Bridge.sol | Fixed limit checking logic and migrated to upgradeable contracts |
| protocol/script/DeployBridge.s.sol | Reformatted deployment script |
| protocol/foundry.toml | Updated remappings for upgradeable contracts |
| .gitmodules | Added upgradeable contracts submodule reference |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| uint256 _version, | ||
| bytes32 _merkleRoot | ||
| ) external initializer { | ||
| __AccessControl_init(); |
There was a problem hiding this comment.
The __AccessControl_init() function is called without the _init_unchained pattern, which could lead to duplicate initialization if this contract is inherited. Consider using __AccessControl_init_unchained() instead, or verify that the initialization pattern is correct for your inheritance hierarchy.
| __AccessControl_init(); | |
| __AccessControl_init_unchained(); |
| mstore(add(ptr, 0x44), to) | ||
|
|
||
| dataHash := keccak256(ptr, 0x64) | ||
| dataHash := keccak256(ptr, lenData) |
There was a problem hiding this comment.
The variable lenData is used but not defined in this assembly block. Based on the context, this should be 0x64 (100 bytes) to match the data structure being hashed. This will cause compilation failure.
There was a problem hiding this comment.
this is obviously untrue since the code compiles
Finding (3.1.1) from the Bridge Audit Report recommended that we specify the version of the transactions being claimed. Otherwise, the merkle tree should be based on resigning all the transactions that havent been claimed so far from the previous round, and would force us to make sure that no transaction that has been claimed is part of the merkle tree.
No description provided.