From 566ad7f654c2e5ddf41cb74f51594436abe082f5 Mon Sep 17 00:00:00 2001 From: vedhavyas Date: Thu, 5 Dec 2024 13:06:44 +0530 Subject: [PATCH 1/3] additional overlay checks --- contracts/pyth-governance-v1.clar | 14 ++++++++++---- contracts/pyth-pnau-decoder-v1.clar | 9 +++++++-- contracts/wormhole/wormhole-core-v2.clar | 4 ++++ 3 files changed, 21 insertions(+), 6 deletions(-) diff --git a/contracts/pyth-governance-v1.clar b/contracts/pyth-governance-v1.clar index 5cd2fdd..07e555e 100644 --- a/contracts/pyth-governance-v1.clar +++ b/contracts/pyth-governance-v1.clar @@ -50,6 +50,8 @@ (define-constant ERR_INVALID_PTGM (err u4007)) ;; Error not standard principal (define-constant ERR_NOT_STANDARD_PRINCIPAL (err u4008)) +;; Error Ptgm overlay bytes +(define-constant ERR_PTGM_CHECK_OVERLAY (err u4009)) (define-data-var governance-data-source { emitter-chain: uint, emitter-address: (buff 32) } @@ -365,7 +367,8 @@ (cursor-target-chain-id (unwrap! (contract-call? 'SP2J933XB2CP2JQ1A4FGN8JA968BBG3NK3EKZ7Q9F.hk-cursor-v2 read-buff-2 (get next cursor-action)) ERR_INVALID_PTGM)) (cursor-body (unwrap! (contract-call? 'SP2J933XB2CP2JQ1A4FGN8JA968BBG3NK3EKZ7Q9F.hk-cursor-v2 read-buff-8192-max (get next cursor-target-chain-id) none) - ERR_INVALID_PTGM))) + ERR_INVALID_PTGM)) + (overlay-check (asserts! (is-eq (get pos (get next cursor-body)) (len ptgm-bytes)) ERR_PTGM_CHECK_OVERLAY))) ;; Check magic bytes (asserts! (is-eq (get value cursor-magic) PTGM_MAGIC) ERR_INVALID_PTGM) ;; Check target-chain-id @@ -389,7 +392,8 @@ (cursor-mantissa (unwrap! (contract-call? 'SP2J933XB2CP2JQ1A4FGN8JA968BBG3NK3EKZ7Q9F.hk-cursor-v2 read-uint-64 (get next cursor-ptgm-body)) ERR_INVALID_ACTION_PAYLOAD)) (cursor-exponent (unwrap! (contract-call? 'SP2J933XB2CP2JQ1A4FGN8JA968BBG3NK3EKZ7Q9F.hk-cursor-v2 read-uint-64 (get next cursor-mantissa)) - ERR_INVALID_ACTION_PAYLOAD))) + ERR_INVALID_ACTION_PAYLOAD)) + (overlay-check (asserts! (is-eq (get pos (get next cursor-exponent)) (len ptgm-body)) ERR_PTGM_CHECK_OVERLAY))) (ok { mantissa: (get value cursor-mantissa), exponent: (get value cursor-exponent) @@ -398,7 +402,8 @@ (define-private (parse-and-verify-stale-price-threshold (ptgm-body (buff 8192))) (let ((cursor-ptgm-body (contract-call? 'SP2J933XB2CP2JQ1A4FGN8JA968BBG3NK3EKZ7Q9F.hk-cursor-v2 new ptgm-body none)) (cursor-stale-price-threshold (unwrap! (contract-call? 'SP2J933XB2CP2JQ1A4FGN8JA968BBG3NK3EKZ7Q9F.hk-cursor-v2 read-uint-64 (get next cursor-ptgm-body)) - ERR_INVALID_ACTION_PAYLOAD))) + ERR_INVALID_ACTION_PAYLOAD)) + (overlay-check (asserts! (is-eq (get pos (get next cursor-stale-price-threshold)) (len ptgm-body)) ERR_PTGM_CHECK_OVERLAY))) (ok (get value cursor-stale-price-threshold)))) (define-private (parse-and-verify-governance-data-source (ptgm-body (buff 8192))) @@ -408,7 +413,8 @@ (cursor-emitter-sequence (unwrap! (contract-call? 'SP2J933XB2CP2JQ1A4FGN8JA968BBG3NK3EKZ7Q9F.hk-cursor-v2 read-uint-64 (get next cursor-emitter-chain)) ERR_INVALID_ACTION_PAYLOAD)) (cursor-emitter-address (unwrap! (contract-call? 'SP2J933XB2CP2JQ1A4FGN8JA968BBG3NK3EKZ7Q9F.hk-cursor-v2 read-buff-32 (get next cursor-emitter-sequence)) - ERR_INVALID_ACTION_PAYLOAD))) + ERR_INVALID_ACTION_PAYLOAD)) + (overlay-check (asserts! (is-eq (get pos (get next cursor-emitter-address)) (len ptgm-body)) ERR_PTGM_CHECK_OVERLAY))) (ok { emitter-chain: (get value cursor-emitter-chain), emitter-sequence: (get value cursor-emitter-sequence), diff --git a/contracts/pyth-pnau-decoder-v1.clar b/contracts/pyth-pnau-decoder-v1.clar index 6d1a3c8..fa4a007 100644 --- a/contracts/pyth-pnau-decoder-v1.clar +++ b/contracts/pyth-pnau-decoder-v1.clar @@ -42,6 +42,8 @@ (define-constant ERR_UNAUTHORIZED_FLOW (err u2404)) ;; Price update not signed by an authorized source (define-constant ERR_UNAUTHORIZED_PRICE_UPDATE (err u2401)) +;; VAA buffer has unused, extra leading bytes (overlay) +(define-constant ERR_OVERLAY_PRESENT (err u2402)) ;;;; Public functions (define-public (decode-and-verify-price-feeds (pnau-bytes (buff 8192)) (wormhole-core-address )) @@ -132,7 +134,7 @@ (define-private (parse-and-verify-prices-updates (bytes (buff 8192)) (merkle-root-hash (buff 20))) (let ((cursor-num-updates (try! (contract-call? 'SP2J933XB2CP2JQ1A4FGN8JA968BBG3NK3EKZ7Q9F.hk-cursor-v2 read-uint-8 { bytes: bytes, pos: u0 }))) (cursor-updates-bytes (contract-call? 'SP2J933XB2CP2JQ1A4FGN8JA968BBG3NK3EKZ7Q9F.hk-cursor-v2 slice (get next cursor-num-updates) none)) - (updates (get result (fold parse-price-info-and-proof cursor-updates-bytes { + (updates-data (fold parse-price-info-and-proof cursor-updates-bytes { result: (list), cursor: { index: u0, @@ -140,12 +142,15 @@ }, bytes: cursor-updates-bytes, limit: (get value cursor-num-updates) - }))) + })) + (updates (get result updates-data)) (merkle-proof-checks-success (get result (fold check-merkle-proof updates { result: true, merkle-root-hash: merkle-root-hash })))) (asserts! merkle-proof-checks-success MERKLE_ROOT_MISMATCH) + ;; Overlay check; 1 is added because 1 byte is used to store "cursor-num-updates" + (asserts! (is-eq (+ u1 (get next-update-index (get cursor updates-data))) (len bytes)) ERR_OVERLAY_PRESENT) (ok updates))) (define-private (check-merkle-proof diff --git a/contracts/wormhole/wormhole-core-v2.clar b/contracts/wormhole/wormhole-core-v2.clar index b72f095..199643b 100644 --- a/contracts/wormhole/wormhole-core-v2.clar +++ b/contracts/wormhole/wormhole-core-v2.clar @@ -70,6 +70,8 @@ (define-constant ERR_GSU_CHECK_EMITTER (err u1305)) ;; First guardian set is not being updated by the deployer (define-constant ERR_NOT_DEPLOYER (err u1306)) +;; Overlay present in vaa bytes +(define-constant ERR_GSU_CHECK_OVERLAY (err u1307)) ;; Guardian set upgrade emitting address (define-constant GSU-EMITTING-ADDRESS 0x0000000000000000000000000000000000000000000000000000000000000004) @@ -147,6 +149,7 @@ ERR_VAA_PARSING_CONSISTENCY_LEVEL)) (cursor-payload (unwrap! (contract-call? 'SP2J933XB2CP2JQ1A4FGN8JA968BBG3NK3EKZ7Q9F.hk-cursor-v2 read-buff-8192-max (get next cursor-consistency-level) none) ERR_VAA_PARSING_PAYLOAD)) + (overlay-check (asserts! (is-eq (get pos (get next cursor-payload)) (len vaa-bytes)) ERR_GSU_CHECK_OVERLAY)) (public-keys-results (fold batch-recover-public-keys (get value cursor-signatures) { @@ -348,6 +351,7 @@ ERR_GSU_PARSING_GUARDIAN_LEN)) (guardians-bytes (unwrap! (contract-call? 'SP2J933XB2CP2JQ1A4FGN8JA968BBG3NK3EKZ7Q9F.hk-cursor-v2 read-buff-8192-max (get next cursor-guardians-count) (some (* (get value cursor-guardians-count) u20))) ERR_GSU_PARSING_GUARDIANS_BYTES)) + (overlay-check (asserts! (is-eq (get pos (get next guardians-bytes)) (len bytes)) ERR_GSU_CHECK_OVERLAY)) (guardians-cues (get result (fold is-guardian-cue (get value guardians-bytes) { cursor: u0, result: (list) }))) (eth-addresses (get result (fold parse-guardian guardians-cues { bytes: (get value guardians-bytes), result: (list) })))) ;; Ensure that this message was emitted from authorized module From 0b863d30e42d5a4dd7fa82f7db4f74a5bec6ad3a Mon Sep 17 00:00:00 2001 From: vedhavyas Date: Thu, 5 Dec 2024 13:24:21 +0530 Subject: [PATCH 2/3] fix L-10 --- contracts/pyth-pnau-decoder-v1.clar | 2 +- unit-tests/pyth/pnau.test.ts | 4 ++-- 2 files changed, 3 insertions(+), 3 deletions(-) diff --git a/contracts/pyth-pnau-decoder-v1.clar b/contracts/pyth-pnau-decoder-v1.clar index fa4a007..4445e31 100644 --- a/contracts/pyth-pnau-decoder-v1.clar +++ b/contracts/pyth-pnau-decoder-v1.clar @@ -117,7 +117,7 @@ ;; Check major version (asserts! (is-eq (get value cursor-version-maj) PYTHNET_MAJOR_VERSION) ERR_VERSION_MAJ) ;; Check minor version - (asserts! (is-eq (get value cursor-version-min) PYTHNET_MINOR_VERSION) ERR_VERSION_MIN) + (asserts! (>= (get value cursor-version-min) PYTHNET_MINOR_VERSION) ERR_VERSION_MIN) ;; Check proof type (asserts! (is-eq (get value cursor-proof-type) u0) ERR_PROOF_TYPE) (ok { diff --git a/unit-tests/pyth/pnau.test.ts b/unit-tests/pyth/pnau.test.ts index 8fe3b27..14fc20e 100644 --- a/unit-tests/pyth/pnau.test.ts +++ b/unit-tests/pyth/pnau.test.ts @@ -564,7 +564,7 @@ describe("pyth-pnau-decoder-v1::decode-and-verify-price-feeds failures", () => { expect(res.result).toBeErr(Cl.uint(2002)); }); - it("should fail if PNAU minor version is incorrect", () => { + it("should not fail if PNAU minor version is above minimum minor due to forward compatibility", () => { let actualPricesUpdates = pyth.buildPriceUpdateBatch([ [pyth.BtcPriceIdentifier, { price: 100n }], ]); @@ -597,7 +597,7 @@ describe("pyth-pnau-decoder-v1::decode-and-verify-price-feeds failures", () => { [Cl.buffer(pnau), executionPlan], sender, ); - expect(res.result).toBeErr(Cl.uint(2003)); + expect(res.result.type).toBe(ClarityType.ResponseOk) }); it("should fail if PNAU proof type version is incorrect", () => { From d74dd20d8ce9f70440a75ed60cf553a0055ae274 Mon Sep 17 00:00:00 2001 From: vedhavyas Date: Thu, 5 Dec 2024 16:58:06 +0530 Subject: [PATCH 3/3] further fixes --- contracts/pyth-governance-v1.clar | 19 +++++++++++-------- contracts/wormhole/wormhole-core-v2.clar | 4 ++-- 2 files changed, 13 insertions(+), 10 deletions(-) diff --git a/contracts/pyth-governance-v1.clar b/contracts/pyth-governance-v1.clar index 07e555e..df4aa6b 100644 --- a/contracts/pyth-governance-v1.clar +++ b/contracts/pyth-governance-v1.clar @@ -392,8 +392,8 @@ (cursor-mantissa (unwrap! (contract-call? 'SP2J933XB2CP2JQ1A4FGN8JA968BBG3NK3EKZ7Q9F.hk-cursor-v2 read-uint-64 (get next cursor-ptgm-body)) ERR_INVALID_ACTION_PAYLOAD)) (cursor-exponent (unwrap! (contract-call? 'SP2J933XB2CP2JQ1A4FGN8JA968BBG3NK3EKZ7Q9F.hk-cursor-v2 read-uint-64 (get next cursor-mantissa)) - ERR_INVALID_ACTION_PAYLOAD)) - (overlay-check (asserts! (is-eq (get pos (get next cursor-exponent)) (len ptgm-body)) ERR_PTGM_CHECK_OVERLAY))) + ERR_INVALID_ACTION_PAYLOAD))) + (asserts! (is-eq (get pos (get next cursor-exponent)) (len ptgm-body)) ERR_PTGM_CHECK_OVERLAY) (ok { mantissa: (get value cursor-mantissa), exponent: (get value cursor-exponent) @@ -402,8 +402,8 @@ (define-private (parse-and-verify-stale-price-threshold (ptgm-body (buff 8192))) (let ((cursor-ptgm-body (contract-call? 'SP2J933XB2CP2JQ1A4FGN8JA968BBG3NK3EKZ7Q9F.hk-cursor-v2 new ptgm-body none)) (cursor-stale-price-threshold (unwrap! (contract-call? 'SP2J933XB2CP2JQ1A4FGN8JA968BBG3NK3EKZ7Q9F.hk-cursor-v2 read-uint-64 (get next cursor-ptgm-body)) - ERR_INVALID_ACTION_PAYLOAD)) - (overlay-check (asserts! (is-eq (get pos (get next cursor-stale-price-threshold)) (len ptgm-body)) ERR_PTGM_CHECK_OVERLAY))) + ERR_INVALID_ACTION_PAYLOAD))) + (asserts! (is-eq (get pos (get next cursor-stale-price-threshold)) (len ptgm-body)) ERR_PTGM_CHECK_OVERLAY) (ok (get value cursor-stale-price-threshold)))) (define-private (parse-and-verify-governance-data-source (ptgm-body (buff 8192))) @@ -413,8 +413,8 @@ (cursor-emitter-sequence (unwrap! (contract-call? 'SP2J933XB2CP2JQ1A4FGN8JA968BBG3NK3EKZ7Q9F.hk-cursor-v2 read-uint-64 (get next cursor-emitter-chain)) ERR_INVALID_ACTION_PAYLOAD)) (cursor-emitter-address (unwrap! (contract-call? 'SP2J933XB2CP2JQ1A4FGN8JA968BBG3NK3EKZ7Q9F.hk-cursor-v2 read-buff-32 (get next cursor-emitter-sequence)) - ERR_INVALID_ACTION_PAYLOAD)) - (overlay-check (asserts! (is-eq (get pos (get next cursor-emitter-address)) (len ptgm-body)) ERR_PTGM_CHECK_OVERLAY))) + ERR_INVALID_ACTION_PAYLOAD))) + (asserts! (is-eq (get pos (get next cursor-emitter-address)) (len ptgm-body)) ERR_PTGM_CHECK_OVERLAY) (ok { emitter-chain: (get value cursor-emitter-chain), emitter-sequence: (get value cursor-emitter-sequence), @@ -426,6 +426,7 @@ (cursor-principal-len (try! (contract-call? 'SP2J933XB2CP2JQ1A4FGN8JA968BBG3NK3EKZ7Q9F.hk-cursor-v2 read-uint-8 (get next cursor-ptgm-body)))) (principal-bytes (contract-call? 'SP2J933XB2CP2JQ1A4FGN8JA968BBG3NK3EKZ7Q9F.hk-cursor-v2 slice (get next cursor-principal-len) (some (get value cursor-principal-len)))) (new-principal (unwrap! (from-consensus-buff? principal principal-bytes) ERR_UNEXPECTED_ACTION_PAYLOAD))) + (asserts! (is-eq (+ (get pos (get next cursor-principal-len)) (get value cursor-principal-len)) (len ptgm-body)) ERR_PTGM_CHECK_OVERLAY) (asserts! (is-standard new-principal) ERR_NOT_STANDARD_PRINCIPAL) (ok new-principal))) @@ -433,7 +434,7 @@ (let ((cursor-ptgm-body (contract-call? 'SP2J933XB2CP2JQ1A4FGN8JA968BBG3NK3EKZ7Q9F.hk-cursor-v2 new ptgm-body none)) (cursor-num-data-sources (try! (contract-call? 'SP2J933XB2CP2JQ1A4FGN8JA968BBG3NK3EKZ7Q9F.hk-cursor-v2 read-uint-8 (get next cursor-ptgm-body)))) (cursor-data-sources-bytes (contract-call? 'SP2J933XB2CP2JQ1A4FGN8JA968BBG3NK3EKZ7Q9F.hk-cursor-v2 slice (get next cursor-num-data-sources) none)) - (data-sources (get result (fold parse-data-source cursor-data-sources-bytes { + (data-sources-bundle (fold parse-data-source cursor-data-sources-bytes { result: (list), cursor: { index: u0, @@ -441,7 +442,9 @@ }, bytes: cursor-data-sources-bytes, limit: (get value cursor-num-data-sources) - })))) + })) + (data-sources (get result data-sources-bundle))) + (asserts! (is-eq (get next-update-index (get cursor data-sources-bundle)) (len cursor-data-sources-bytes)) ERR_PTGM_CHECK_OVERLAY) (ok data-sources))) (define-private (parse-data-source diff --git a/contracts/wormhole/wormhole-core-v2.clar b/contracts/wormhole/wormhole-core-v2.clar index 199643b..1609480 100644 --- a/contracts/wormhole/wormhole-core-v2.clar +++ b/contracts/wormhole/wormhole-core-v2.clar @@ -149,13 +149,13 @@ ERR_VAA_PARSING_CONSISTENCY_LEVEL)) (cursor-payload (unwrap! (contract-call? 'SP2J933XB2CP2JQ1A4FGN8JA968BBG3NK3EKZ7Q9F.hk-cursor-v2 read-buff-8192-max (get next cursor-consistency-level) none) ERR_VAA_PARSING_PAYLOAD)) - (overlay-check (asserts! (is-eq (get pos (get next cursor-payload)) (len vaa-bytes)) ERR_GSU_CHECK_OVERLAY)) (public-keys-results (fold batch-recover-public-keys (get value cursor-signatures) { message-hash: vaa-body-hash, value: (list) }))) + (asserts! (is-eq (get pos (get next cursor-payload)) (len vaa-bytes)) ERR_GSU_CHECK_OVERLAY) (print { payload: (get value cursor-payload) }) (ok { vaa: { @@ -351,9 +351,9 @@ ERR_GSU_PARSING_GUARDIAN_LEN)) (guardians-bytes (unwrap! (contract-call? 'SP2J933XB2CP2JQ1A4FGN8JA968BBG3NK3EKZ7Q9F.hk-cursor-v2 read-buff-8192-max (get next cursor-guardians-count) (some (* (get value cursor-guardians-count) u20))) ERR_GSU_PARSING_GUARDIANS_BYTES)) - (overlay-check (asserts! (is-eq (get pos (get next guardians-bytes)) (len bytes)) ERR_GSU_CHECK_OVERLAY)) (guardians-cues (get result (fold is-guardian-cue (get value guardians-bytes) { cursor: u0, result: (list) }))) (eth-addresses (get result (fold parse-guardian guardians-cues { bytes: (get value guardians-bytes), result: (list) })))) + (asserts! (is-eq (get pos (get next guardians-bytes)) (len bytes)) ERR_GSU_CHECK_OVERLAY) ;; Ensure that this message was emitted from authorized module (asserts! (is-eq (get value cursor-module) 0x00000000000000000000000000000000000000000000000000000000436f7265) ERR_GSU_CHECK_MODULE)