feat(contract): implement FraudProofRouter
#4927
Conversation
|
| // store origin => signer => merkleTree => digest => {timestamp, fraudType} | ||
| mapping(uint32 origin => mapping(bytes32 signer => mapping(bytes32 merkleTree => mapping(bytes32 digest => Attribution)))) | ||
| public fraudAttributions; |
There was a problem hiding this comment.
this is pretty redundant with the internals of the AttributeCheckpointFraud contract
There was a problem hiding this comment.
the remote chain doesn't know the internals of AttributeCheckpointFraud and it becomes easier for the client sdk or contract.
There was a problem hiding this comment.
can we somehow merge these 2 contracts?
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
There was a problem hiding this comment.
Olympix Integrated Security found more than 20 potential problems in the proposed changes. Check the Files changed tab for more details.
| ( | ||
| address signer, | ||
| bytes32 merkleTree, | ||
| bytes32 digest, |
Check notice
Code scanning / Olympix Integrated Security
Local variables in test functions are not properly fuzzed, potentially reducing the effectiveness of property-based testing. For more information, visit: http://detectors.olympixdevsectools.com/article/web3-vulnerability/unfuzzed-local-variables Low
| @@ -0,0 +1,128 @@ | |||
| // 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
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #4927 +/- ##
==========================================
+ Coverage 77.53% 77.68% +0.14%
==========================================
Files 103 105 +2
Lines 2110 2142 +32
Branches 190 194 +4
==========================================
+ Hits 1636 1664 +28
- Misses 453 456 +3
- Partials 21 22 +1
|
| // store origin => signer => merkleTree => digest => {timestamp, fraudType} | ||
| mapping(uint32 origin => mapping(bytes32 signer => mapping(bytes32 merkleTree => mapping(bytes32 digest => Attribution)))) | ||
| public fraudAttributions; |
There was a problem hiding this comment.
can we somehow merge these 2 contracts?
| Attribution attribution | ||
| ); | ||
|
|
||
| event LocalFraudProofReceived( |
Check warning
Code scanning / Olympix Integrated Security
Test functions fail to assert the emission of expected events, potentially missing critical contract behaviors. For more information, visit: http://detectors.olympixdevsectools.com/article/web3-vulnerability/missing-events-assertion Medium
| _digest | ||
| ); | ||
|
|
||
| require(attribution.timestamp != 0, "Attribution does not exist"); |
Check warning
Code scanning / Olympix Integrated Security
Test functions fail to verify specific revert reasons, potentially missing important contract behavior validation. For more information, visit: http://detectors.olympixdevsectools.com/article/web3-vulnerability/missing-revert-reason-tests Medium
| Attribution memory attribution = attributeCheckpointFraud.attributions( | ||
| _signer, | ||
| _digest | ||
| ); |
There was a problem hiding this comment.
I thought you would literally merge these two contracts
as it stands the storage is still redundant
There was a problem hiding this comment.
I thought you said to merge the mapping now here: https://discord.com/channels/935678348330434570/1317101275388837918/1317154364887928892
There was a problem hiding this comment.
I did, but you still have 2 mappings
There was a problem hiding this comment.
the AttributeCheckpointFraud.attributions mapping has redundant information with this contract's fraudAttributions[chainId]
I am proposing merging these contracts, having a single mapping, and a single storage location for this information
| AttributeCheckpointFraud public immutable attributeCheckpointFraud; | ||
|
|
||
| // Mapping to store the fraud attributions for a given origin, signer, and digest for easy access for client contracts to aide slashing | ||
| mapping(uint32 origin => mapping(address signer => mapping(bytes32 digest => Attribution))) |
Check warning
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 Medium
| * @param _digest The digest associated with the fraud. | ||
| * @return The message ID of the sent fraud proof. | ||
| */ | ||
| function sendFraudProof( |
Check failure
Code scanning / Olympix Integrated Security
Modifying state after making an external call may allow for reentrancy attacks. For more information, visit: http://detectors.olympixdevsectools.com/article/web3-vulnerability/reentrancy Critical
| * @param _digest The digest associated with the fraud. | ||
| * @return The message ID of the sent fraud proof. | ||
| */ | ||
| function sendFraudProof( |
Check notice
Code scanning / Olympix Integrated Security
Reentrant functions which emit events after making an external call may lead to out-of-order events. For more information, visit: http://detectors.olympixdevsectools.com/article/web3-vulnerability/reentrancy-events Low
| bytes calldata _message | ||
| ) internal override { | ||
| ( | ||
| address signer, |
Check notice
Code scanning / Olympix Integrated Security
Local variables in test functions are not properly fuzzed, potentially reducing the effectiveness of property-based testing. For more information, visit: http://detectors.olympixdevsectools.com/article/web3-vulnerability/unfuzzed-local-variables Low
| ( | ||
| address signer, | ||
| bytes32 digest, | ||
| Attribution memory attribution |
Check notice
Code scanning / Olympix Integrated Security
Local variables in test functions are not properly fuzzed, potentially reducing the effectiveness of property-based testing. For more information, visit: http://detectors.olympixdevsectools.com/article/web3-vulnerability/unfuzzed-local-variables Low
Description
AttributeCheckpointFraudon source and storing them on the router on destination for a client contract like Symbiotic/Eigenlayer to read from.Drive-by changes
Related issues
FraudProofRouterto relay fraud proof attribution results between chains #4324Backward compatibility
Testing