Skip to content

Commit

Permalink
fix!: bug of verifyMultiRowRootsToDataRootTupleRoot (#307)
Browse files Browse the repository at this point in the history
The original `_root` parameter is not verified before being used in
`verifyMultiRowRootsToDataRootTupleRootProof`, it will allow arbitrary
`_rowRoots` to be proved valid.

<!-- This is an auto-generated comment: release notes by coderabbit.ai
-->
## Summary by CodeRabbit

- **Refactor**
- Enhanced data integrity checks by updating the verification process in
the application.
<!-- end of auto-generated comment: release notes by coderabbit.ai -->
  • Loading branch information
zhiqiangxu authored May 3, 2024
1 parent 6b5839d commit 688b3a1
Show file tree
Hide file tree
Showing 3 changed files with 15 additions and 29 deletions.
21 changes: 9 additions & 12 deletions src/lib/verifier/DAVerifier.sol
Original file line number Diff line number Diff line change
Expand Up @@ -78,17 +78,16 @@ library DAVerifier {
/// @notice Verifies that the shares, which were posted to Celestia, were committed to by the Blobstream smart contract.
/// @param _bridge The Blobstream smart contract instance.
/// @param _sharesProof The proof of the shares to the data root tuple root.
/// @param _root The data root of the block that contains the shares.
/// @return `true` if the proof is valid, `false` otherwise.
/// @return an error code if the proof is invalid, ErrorCodes.NoError otherwise.
function verifySharesToDataRootTupleRoot(IDAOracle _bridge, SharesProof memory _sharesProof, bytes32 _root)
function verifySharesToDataRootTupleRoot(IDAOracle _bridge, SharesProof memory _sharesProof)
internal
view
returns (bool, ErrorCodes)
{
// checking that the data root was committed to by the Blobstream smart contract.
(bool success, ErrorCodes errorCode) = verifyMultiRowRootsToDataRootTupleRoot(
_bridge, _sharesProof.rowRoots, _sharesProof.rowProofs, _sharesProof.attestationProof, _root
_bridge, _sharesProof.rowRoots, _sharesProof.rowProofs, _sharesProof.attestationProof
);
if (!success) {
return (false, errorCode);
Expand All @@ -100,7 +99,7 @@ library DAVerifier {
_sharesProof.namespace,
_sharesProof.rowRoots,
_sharesProof.rowProofs,
_root
_sharesProof.attestationProof.tuple.dataRoot
);

return (valid, error);
Expand Down Expand Up @@ -164,15 +163,13 @@ library DAVerifier {
/// @param _bridge The Blobstream smart contract instance.
/// @param _rowRoot The row/column root to be proven.
/// @param _rowProof The proof of the row/column root to the data root.
/// @param _root The data root of the block that contains the row.
/// @return `true` if the proof is valid, `false` otherwise.
/// @return an error code if the proof is invalid, ErrorCodes.NoError otherwise.
function verifyRowRootToDataRootTupleRoot(
IDAOracle _bridge,
NamespaceNode memory _rowRoot,
BinaryMerkleProof memory _rowProof,
AttestationProof memory _attestationProof,
bytes32 _root
AttestationProof memory _attestationProof
) internal view returns (bool, ErrorCodes) {
// checking that the data root was committed to by the Blobstream smart contract
if (
Expand All @@ -183,7 +180,8 @@ library DAVerifier {
return (false, ErrorCodes.InvalidDataRootTupleToDataRootTupleRootProof);
}

(bool valid, ErrorCodes error) = verifyRowRootToDataRootTupleRootProof(_rowRoot, _rowProof, _root);
(bool valid, ErrorCodes error) =
verifyRowRootToDataRootTupleRootProof(_rowRoot, _rowProof, _attestationProof.tuple.dataRoot);

return (valid, error);
}
Expand Down Expand Up @@ -213,15 +211,13 @@ library DAVerifier {
/// @param _bridge The Blobstream smart contract instance.
/// @param _rowRoots The set of row/column roots to be proved.
/// @param _rowProofs The set of proofs of the _rowRoots in the same order.
/// @param _root The data root of the block that contains the rows.
/// @return `true` if the proof is valid, `false` otherwise.
/// @return an error code if the proof is invalid, ErrorCodes.NoError otherwise.
function verifyMultiRowRootsToDataRootTupleRoot(
IDAOracle _bridge,
NamespaceNode[] memory _rowRoots,
BinaryMerkleProof[] memory _rowProofs,
AttestationProof memory _attestationProof,
bytes32 _root
AttestationProof memory _attestationProof
) internal view returns (bool, ErrorCodes) {
// checking that the data root was committed to by the Blobstream smart contract
if (
Expand All @@ -233,7 +229,8 @@ library DAVerifier {
}

// checking that the rows roots commit to the data root.
(bool valid, ErrorCodes error) = verifyMultiRowRootsToDataRootTupleRootProof(_rowRoots, _rowProofs, _root);
(bool valid, ErrorCodes error) =
verifyMultiRowRootsToDataRootTupleRootProof(_rowRoots, _rowProofs, _attestationProof.tuple.dataRoot);

return (valid, error);
}
Expand Down
14 changes: 4 additions & 10 deletions src/lib/verifier/test/DAVerifier.t.sol
Original file line number Diff line number Diff line change
Expand Up @@ -126,8 +126,7 @@ contract DAVerifierTest is DSTest {
SharesProof memory sharesProof =
SharesProof(_data, _shareProofs, fixture.getNamespace(), _rowRoots, _rowProofs, attestationProof);

(bool valid, DAVerifier.ErrorCodes errorCode) =
DAVerifier.verifySharesToDataRootTupleRoot(bridge, sharesProof, fixture.dataRoot());
(bool valid, DAVerifier.ErrorCodes errorCode) = DAVerifier.verifySharesToDataRootTupleRoot(bridge, sharesProof);
assertTrue(valid);
assertEq(uint8(errorCode), uint8(DAVerifier.ErrorCodes.NoError));
}
Expand All @@ -138,11 +137,7 @@ contract DAVerifierTest is DSTest {
);

(bool valid, DAVerifier.ErrorCodes errorCode) = DAVerifier.verifyRowRootToDataRootTupleRoot(
bridge,
fixture.getFirstRowRootNode(),
fixture.getRowRootToDataRootProof(),
attestationProof,
fixture.dataRoot()
bridge, fixture.getFirstRowRootNode(), fixture.getRowRootToDataRootProof(), attestationProof
);
assertTrue(valid);
assertEq(uint8(errorCode), uint8(DAVerifier.ErrorCodes.NoError));
Expand All @@ -159,9 +154,8 @@ contract DAVerifierTest is DSTest {
fixture.dataRootTupleRootNonce(), fixture.getDataRootTuple(), fixture.getDataRootTupleProof()
);

(bool valid, DAVerifier.ErrorCodes errorCode) = DAVerifier.verifyMultiRowRootsToDataRootTupleRoot(
bridge, _rowRoots, _rowProofs, attestationProof, fixture.dataRoot()
);
(bool valid, DAVerifier.ErrorCodes errorCode) =
DAVerifier.verifyMultiRowRootsToDataRootTupleRoot(bridge, _rowRoots, _rowProofs, attestationProof);
assertTrue(valid);
assertEq(uint8(errorCode), uint8(DAVerifier.ErrorCodes.NoError));
}
Expand Down
9 changes: 2 additions & 7 deletions src/lib/verifier/test/RollupInclusionProofs.t.sol
Original file line number Diff line number Diff line change
Expand Up @@ -268,11 +268,7 @@ contract RollupInclusionProofTest is DSTest {
);
bool success;
(success, error) = DAVerifier.verifyRowRootToDataRootTupleRoot(
bridge,
fixture.getFirstRowRootNode(),
fixture.getRowRootToDataRootProof(),
attestationProof,
fixture.dataRoot()
bridge, fixture.getFirstRowRootNode(), fixture.getRowRootToDataRootProof(), attestationProof
);
assertTrue(success);
assertEq(uint8(error), uint8(DAVerifier.ErrorCodes.NoError));
Expand Down Expand Up @@ -323,8 +319,7 @@ contract RollupInclusionProofTest is DSTest {

// let's authenticate the share proof to the data root tuple root to be sure that
// the square size is valid.
(bool success, DAVerifier.ErrorCodes error) =
DAVerifier.verifySharesToDataRootTupleRoot(bridge, shareProof, fixture.dataRoot());
(bool success, DAVerifier.ErrorCodes error) = DAVerifier.verifySharesToDataRootTupleRoot(bridge, shareProof);
assertTrue(success);
assertEq(uint8(error), uint8(DAVerifier.ErrorCodes.NoError));

Expand Down

0 comments on commit 688b3a1

Please sign in to comment.