code423n4 commented 2 years ago

Lines of code

https://github.com/code-423n4/2022-10-blur/blob/2fdaa6e13b544c8c11d1c022a575f16c3a72e3bf/contracts/BlurExchange.sol#L375-L393

Vulnerability details

Impact

Bulk orders are not correctly signed and don't work as intended

Proof of Concept

BlurExchange.sol#L386-L392

} else if (signatureVersion == SignatureVersion.Bulk) {
    /* If the signature was a bulk listing the merkle path musted be unpacked before the oracle signature. */
    (bytes32[] memory merklePath, uint8 _v, bytes32 _r, bytes32 _s) = abi.decode(extraSignature, (bytes32[], uint8, bytes32, bytes32));
    v = _v; r = _r; s = _s;
}

return _recover(oracleHash, v, r, s) == oracle;

When the order uses a bulk oracle signature it tries to verify the order hash rather than the root hash so it won't work as intended.

Tools Used

Manual Review

Recommended Mitigation Steps

Modify logic to compute and hash the order root for bulk listings:

} else if (signatureVersion == SignatureVersion.Bulk) {
    /* If the signature was a bulk listing the merkle path musted be unpacked before the oracle signature. */
    (bytes32[] memory merklePath, uint8 _v, bytes32 _r, bytes32 _s) = abi.decode(extraSignature, (bytes32[], uint8, bytes32, bytes32));

+   bytes32 computedRoot = MerkleVerifier._computeRoot(oracleHash, merklePath);
+   oracleHash = _hashToSignRoot(computedRoot);

    v = _v; r = _r; s = _s;
}

return _recover(oracleHash, v, r, s) == oracle;

blur-io-toad commented 2 years ago

The oracle signs the individual order, not the bulk listing.

GalloDaSballo commented 2 years ago

The finding doesn't seem to show how the signature scheme is broken.

From my perspective extraSignature is used for the validation from the user trade

    } else if (signatureVersion == SignatureVersion.Bulk) {
        /* Bulk-listing authentication: Merkle root of orders signed by trader */
        (bytes32[] memory merklePath) = abi.decode(extraSignature, (bytes32[]));

https://github.com/code-423n4/2022-10-blur/blob/431d53c3fe4f1aa697f0f3466691232cde86749d/contracts/BlurExchange.sol#L387-L390

And the remaining field for the oracleTrade

            v = _v; r = _r; s = _s;
        }

        return _recover(oracleHash, v, r, s) == oracle;

For that reason I think the finding is invalid (would also recommend the Warden to submit an instances that proves their point next time)

code-423n4 / 2022-10-blur-findings