From 91313adc8eebfcfbb193632558aa1c3a9186572e Mon Sep 17 00:00:00 2001 From: CedarMist <134699267+CedarMist@users.noreply.github.com> Date: Mon, 1 Jul 2024 13:47:59 +0100 Subject: [PATCH 1/9] contracts: fix CBOR unsigned int decoding in Subcall.sol fixes #323 --- contracts/CHANGELOG.md | 6 +++++ contracts/contracts/Subcall.sol | 48 +++++++++++++++++++++++++++++---- contracts/package.json | 2 +- contracts/test/subcall.ts | 23 +++++++++++----- 4 files changed, 66 insertions(+), 13 deletions(-) diff --git a/contracts/CHANGELOG.md b/contracts/CHANGELOG.md index 25a91f28..529d7464 100644 --- a/contracts/CHANGELOG.md +++ b/contracts/CHANGELOG.md @@ -6,6 +6,12 @@ The format is inspired by [Keep a Changelog]. [Keep a Changelog]: https://keepachangelog.com/en/1.0.0/ +## 0.2.9 (2024-07-01) + +### Fixed + + * CBOR unsigned integer decoding in Subcall.sol + ## 0.2.8 (2024-03-15) ### Fixed diff --git a/contracts/contracts/Subcall.sol b/contracts/contracts/Subcall.sol index a87f2a77..8bd167d3 100644 --- a/contracts/contracts/Subcall.sol +++ b/contracts/contracts/Subcall.sol @@ -52,7 +52,7 @@ library Subcall { error InvalidMap(); /// While parsing CBOR structure, data length was unexpected - error InvalidLength(); + error InvalidLength(uint); /// Invalid receipt ID error InvalidReceiptId(); @@ -63,6 +63,12 @@ library Subcall { /// CBOR parser expected a key, but it was not found in the map! error MissingKey(); + /// Valid cannot be parsed as a uint + error InvalidUintPrefix(uint8); + + /// Unsigned integer of unknown size + error InvalidUintSize(uint8); + /** * @notice Submit a native message to the Oasis runtime layer. Messages * which re-enter the EVM module are forbidden: `evm.*`. @@ -182,11 +188,43 @@ library Subcall { pure returns (uint256 newOffset, uint256 value) { - if (result[offset] & 0x40 != 0x40) revert InvalidLength(); + uint8 prefix = uint8(result[offset]); + uint len; - uint256 len = uint8(result[offset++]) ^ 0x40; + if( prefix <= 0x17 ) { + return (offset + 1, prefix); + } + // byte array, parsed as a big-endian integer + else if ( prefix & 0x40 == 0x40 ) + { + len = uint8(result[offset++]) ^ 0x40; + } + // unsigned integer, CBOR encoded + else if( prefix & 0x10 == 0x10 ) + { + if( prefix == 0x18 ) { + len = 1; + } + else if( prefix == 0x19 ) { + len = 2; + } + else if( prefix == 0x1a ) { + len = 4; + } + else if( prefix == 0x1b ) { + len = 8; + } + else { + revert InvalidUintSize(prefix); + } + offset += 1; + } + // Unknown... + else { + revert InvalidUintPrefix(prefix); + } - if (len >= 0x20) revert InvalidLength(); + if (len >= 0x20) revert InvalidLength(len); assembly { value := mload(add(add(0x20, result), offset)) @@ -321,7 +359,7 @@ library Subcall { // Delegation succeeded, decode number of shares. uint8 sharesLen = uint8(result[8]) & 0x1f; // Assume shares field is never greater than 16 bytes. - if (9 + sharesLen != result.length) revert InvalidLength(); + if (9 + sharesLen != result.length) revert InvalidLength(sharesLen); for (uint256 offset = 0; offset < sharesLen; offset++) { uint8 v = uint8(result[9 + offset]); diff --git a/contracts/package.json b/contracts/package.json index c29ed6eb..86df039b 100644 --- a/contracts/package.json +++ b/contracts/package.json @@ -1,6 +1,6 @@ { "name": "@oasisprotocol/sapphire-contracts", - "version": "0.2.8", + "version": "0.2.9", "license": "Apache-2.0", "description": "Solidity smart contract library for confidential contract development", "homepage": "https://github.com/oasisprotocol/sapphire-paratime/tree/main/contracts", diff --git a/contracts/test/subcall.ts b/contracts/test/subcall.ts index 03ee51ee..9c1f9f86 100644 --- a/contracts/test/subcall.ts +++ b/contracts/test/subcall.ts @@ -78,7 +78,9 @@ async function ensureBalance( await resp.wait(); } const newBalance = await provider.getBalance(address); - expect(newBalance).eq(initialBalance); + expect(newBalance).gte(initialBalance); + + return newBalance; } function decodeResult(receipt: ContractTransactionReceipt) { @@ -240,8 +242,7 @@ describe('Subcall', () => { ); // Ensure contract has an initial balance, above minimum delegation amount - const initialBalance = parseEther('100'); - await ensureBalance(contract, initialBalance, owner); + await ensureBalance(contract, parseEther('100'), owner); // Perform delegation, and request a receipt let receiptId = randomInt(2 ** 32, 2 ** 32 * 2); @@ -283,17 +284,25 @@ describe('Subcall', () => { // Retrieve UndelegateStart receipt tx = await contract.testTakeReceipt(2, nextReceiptId); receipt = await tx.wait(); - result = cborg.decode(getBytes((receipt?.logs![0] as EventLog).args!.data)); + let resultBytes = (receipt?.logs![0] as EventLog).args!.data; + result = cborg.decode(getBytes(resultBytes)); expect(result.receipt).eq(nextReceiptId); + // Try decoding undelegate start receipt + const undelegateDecoded = await contract.testDecodeReceiptUndelegateStart(resultBytes); + expect(undelegateDecoded[1]).eq(result.receipt); + + const initialContractBalance = await ethers.provider.getBalance(await contract.getAddress()) + await dockerSkipEpochs({ targetEpoch: result.epoch }); - // Retrieve UndelegateStart receipt + // Retrieve UndelegateDone receipt tx = await contract.testTakeReceipt(3, result.receipt); receipt = await tx.wait(); - result = cborg.decode(getBytes((receipt?.logs![0] as EventLog).args!.data)); + resultBytes = (receipt?.logs![0] as EventLog).args!.data; + result = cborg.decode(getBytes(resultBytes)); expect(await ethers.provider.getBalance(await contract.getAddress())).eq( - parseEther('100'), + initialContractBalance + parseEther('100'), ); }); From 0495f64521412ae3c5865bf539c81ad2d142b677 Mon Sep 17 00:00:00 2001 From: CedarMist <134699267+CedarMist@users.noreply.github.com> Date: Mon, 1 Jul 2024 14:23:16 +0100 Subject: [PATCH 2/9] lint fixes --- contracts/contracts/Subcall.sol | 26 ++++++++++---------------- contracts/test/subcall.ts | 8 ++++++-- 2 files changed, 16 insertions(+), 18 deletions(-) diff --git a/contracts/contracts/Subcall.sol b/contracts/contracts/Subcall.sol index 8bd167d3..d004e6ad 100644 --- a/contracts/contracts/Subcall.sol +++ b/contracts/contracts/Subcall.sol @@ -52,7 +52,7 @@ library Subcall { error InvalidMap(); /// While parsing CBOR structure, data length was unexpected - error InvalidLength(uint); + error InvalidLength(uint256); /// Invalid receipt ID error InvalidReceiptId(); @@ -189,32 +189,26 @@ library Subcall { returns (uint256 newOffset, uint256 value) { uint8 prefix = uint8(result[offset]); - uint len; + uint256 len; - if( prefix <= 0x17 ) { + if (prefix <= 0x17) { return (offset + 1, prefix); } // byte array, parsed as a big-endian integer - else if ( prefix & 0x40 == 0x40 ) - { + else if (prefix & 0x40 == 0x40) { len = uint8(result[offset++]) ^ 0x40; } // unsigned integer, CBOR encoded - else if( prefix & 0x10 == 0x10 ) - { - if( prefix == 0x18 ) { + else if (prefix & 0x10 == 0x10) { + if (prefix == 0x18) { len = 1; - } - else if( prefix == 0x19 ) { + } else if (prefix == 0x19) { len = 2; - } - else if( prefix == 0x1a ) { + } else if (prefix == 0x1a) { len = 4; - } - else if( prefix == 0x1b ) { + } else if (prefix == 0x1b) { len = 8; - } - else { + } else { revert InvalidUintSize(prefix); } offset += 1; diff --git a/contracts/test/subcall.ts b/contracts/test/subcall.ts index 9c1f9f86..3d75dfee 100644 --- a/contracts/test/subcall.ts +++ b/contracts/test/subcall.ts @@ -289,10 +289,14 @@ describe('Subcall', () => { expect(result.receipt).eq(nextReceiptId); // Try decoding undelegate start receipt - const undelegateDecoded = await contract.testDecodeReceiptUndelegateStart(resultBytes); + const undelegateDecoded = await contract.testDecodeReceiptUndelegateStart( + resultBytes, + ); expect(undelegateDecoded[1]).eq(result.receipt); - const initialContractBalance = await ethers.provider.getBalance(await contract.getAddress()) + const initialContractBalance = await ethers.provider.getBalance( + await contract.getAddress(), + ); await dockerSkipEpochs({ targetEpoch: result.epoch }); From d32156391c3926a13d6cec369d78c70efa34a56b Mon Sep 17 00:00:00 2001 From: CedarMist <134699267+CedarMist@users.noreply.github.com> Date: Mon, 1 Jul 2024 16:58:22 +0100 Subject: [PATCH 3/9] contracts: ignore unknown keys in CBOR receipt responses --- contracts/contracts/Subcall.sol | 6 ------ 1 file changed, 6 deletions(-) diff --git a/contracts/contracts/Subcall.sol b/contracts/contracts/Subcall.sol index d004e6ad..928aae97 100644 --- a/contracts/contracts/Subcall.sol +++ b/contracts/contracts/Subcall.sol @@ -299,9 +299,6 @@ library Subcall { (offset, endReceipt) = _parseCBORUint64(result, offset); hasReceipt = true; - } else { - // TODO: skip unknown keys & values? For forward compatibility - revert InvalidKey(); } } @@ -328,9 +325,6 @@ library Subcall { (offset, amount) = _parseCBORUint128(result, offset); hasAmount = true; - } else { - // TODO: skip unknown keys & values? For forward compatibility - revert InvalidKey(); } } From ad9d663026f39b3a2143a6ea972be34fe38c9264 Mon Sep 17 00:00:00 2001 From: CedarMist <134699267+CedarMist@users.noreply.github.com> Date: Tue, 2 Jul 2024 12:13:12 +0100 Subject: [PATCH 4/9] Update contracts/contracts/Subcall.sol MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Co-authored-by: Matevž Jekovec --- contracts/contracts/Subcall.sol | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/contracts/contracts/Subcall.sol b/contracts/contracts/Subcall.sol index 928aae97..26fb264e 100644 --- a/contracts/contracts/Subcall.sol +++ b/contracts/contracts/Subcall.sol @@ -198,7 +198,7 @@ library Subcall { else if (prefix & 0x40 == 0x40) { len = uint8(result[offset++]) ^ 0x40; } - // unsigned integer, CBOR encoded + // Unsigned integer, CBOR encoded. else if (prefix & 0x10 == 0x10) { if (prefix == 0x18) { len = 1; From 45dca915060bedd1e25806470cdaa317553eb32d Mon Sep 17 00:00:00 2001 From: CedarMist <134699267+CedarMist@users.noreply.github.com> Date: Tue, 2 Jul 2024 12:13:17 +0100 Subject: [PATCH 5/9] Update contracts/contracts/Subcall.sol MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Co-authored-by: Matevž Jekovec --- contracts/contracts/Subcall.sol | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/contracts/contracts/Subcall.sol b/contracts/contracts/Subcall.sol index 26fb264e..1f5773c4 100644 --- a/contracts/contracts/Subcall.sol +++ b/contracts/contracts/Subcall.sol @@ -194,7 +194,7 @@ library Subcall { if (prefix <= 0x17) { return (offset + 1, prefix); } - // byte array, parsed as a big-endian integer + // Byte array, parsed as a big-endian integer. else if (prefix & 0x40 == 0x40) { len = uint8(result[offset++]) ^ 0x40; } From a7765148cda77137460c45d4daae527dc7a4e7ff Mon Sep 17 00:00:00 2001 From: CedarMist <134699267+CedarMist@users.noreply.github.com> Date: Tue, 2 Jul 2024 19:56:49 +0100 Subject: [PATCH 6/9] contracts: fixed spelling error in comment --- contracts/contracts/Subcall.sol | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/contracts/contracts/Subcall.sol b/contracts/contracts/Subcall.sol index 1f5773c4..c165d3d0 100644 --- a/contracts/contracts/Subcall.sol +++ b/contracts/contracts/Subcall.sol @@ -63,7 +63,7 @@ library Subcall { /// CBOR parser expected a key, but it was not found in the map! error MissingKey(); - /// Valid cannot be parsed as a uint + /// Value cannot be parsed as a uint error InvalidUintPrefix(uint8); /// Unsigned integer of unknown size From 41c4047bb49933213be2786b46b30187b3d5ddca Mon Sep 17 00:00:00 2001 From: Matej Lubej Date: Mon, 29 Jul 2024 19:36:44 +0200 Subject: [PATCH 7/9] contracts: Handle receiptId not found --- contracts/contracts/Subcall.sol | 5 +++++ 1 file changed, 5 insertions(+) diff --git a/contracts/contracts/Subcall.sol b/contracts/contracts/Subcall.sol index c165d3d0..d3f2ac50 100644 --- a/contracts/contracts/Subcall.sol +++ b/contracts/contracts/Subcall.sol @@ -176,6 +176,11 @@ library Subcall { (uint64, bytes) ); + // 0xf6 = null, returns null in case receiptId not found + if (result[0] == 0xf6) { + revert InvalidReceiptId(); + } + if (status != 0) { revert ConsensusTakeReceiptError(status, string(result)); } From 9641422a70f24b9ccb25d7ea4bb28758d7e0dab3 Mon Sep 17 00:00:00 2001 From: Matej Lubej Date: Tue, 30 Jul 2024 08:08:52 +0200 Subject: [PATCH 8/9] contracts: fix uint256 CBOR parsing --- contracts/contracts/Subcall.sol | 7 ++++++- 1 file changed, 6 insertions(+), 1 deletion(-) diff --git a/contracts/contracts/Subcall.sol b/contracts/contracts/Subcall.sol index d3f2ac50..609322a5 100644 --- a/contracts/contracts/Subcall.sol +++ b/contracts/contracts/Subcall.sol @@ -199,6 +199,11 @@ library Subcall { if (prefix <= 0x17) { return (offset + 1, prefix); } + // Byte array(uint256), parsed as a big-endian integer. + else if (prefix == 0x58) { + len = uint8(result[++offset]); + offset++; + } // Byte array, parsed as a big-endian integer. else if (prefix & 0x40 == 0x40) { len = uint8(result[offset++]) ^ 0x40; @@ -223,7 +228,7 @@ library Subcall { revert InvalidUintPrefix(prefix); } - if (len >= 0x20) revert InvalidLength(len); + if (len > 0x20) revert InvalidLength(len); assembly { value := mload(add(add(0x20, result), offset)) From fbca43d0ed7e9d4d3cf23661f43373bdea4e1e79 Mon Sep 17 00:00:00 2001 From: Matej Lubej Date: Tue, 30 Jul 2024 08:13:08 +0200 Subject: [PATCH 9/9] tests: Add parseCBORUint tests --- contracts/contracts/tests/SubcallTests.sol | 8 ++ contracts/test/subcall.ts | 110 ++++++++++++++++++++- 2 files changed, 117 insertions(+), 1 deletion(-) diff --git a/contracts/contracts/tests/SubcallTests.sol b/contracts/contracts/tests/SubcallTests.sol index 98377142..6e6d649d 100644 --- a/contracts/contracts/tests/SubcallTests.sol +++ b/contracts/contracts/tests/SubcallTests.sol @@ -108,4 +108,12 @@ contract SubcallTests { function testConsensusWithdraw(StakingAddress to, uint128 value) external { Subcall.consensusWithdraw(to, value); } + + function testParseCBORUint(bytes memory result, uint256 offset) + external + pure + returns (uint256, uint256) + { + return Subcall._parseCBORUint(result, offset); + } } diff --git a/contracts/test/subcall.ts b/contracts/test/subcall.ts index 3d75dfee..76dd38be 100644 --- a/contracts/test/subcall.ts +++ b/contracts/test/subcall.ts @@ -106,7 +106,13 @@ describe('Subcall', () => { let provider: Provider; before(async () => { - const factory = await ethers.getContractFactory('SubcallTests'); + const subcallFactory = await ethers.getContractFactory('Subcall'); + const subcallLib = await subcallFactory.deploy(); + await subcallLib.waitForDeployment(); + + const factory = await ethers.getContractFactory('SubcallTests', { + libraries: { Subcall: await subcallLib.getAddress() }, + }); contract = (await factory.deploy({ value: parseEther('1.0'), })) as unknown as SubcallTests; @@ -363,4 +369,106 @@ describe('Subcall', () => { expect(shares).eq(toBigInt(num)); } }); + + describe('Should successfully parse CBOR uint/s', () => { + it('Should successfully parse CBOR uint8', async () => { + const MAX_SAFE_UINT8 = 255n; + + // bytes = 0x18FF + const bytes = cborg.encode(MAX_SAFE_UINT8); + + const [newOffset, parsedCborUint] = await contract.testParseCBORUint( + bytes, + 0, + ); + + expect(parsedCborUint).eq(MAX_SAFE_UINT8); + expect(newOffset).eq(1 + 1); + }); + + it('Should successfully parse CBOR uint16', async () => { + const MAX_SAFE_UINT16 = 65535n; + + // bytes = 0x19FFFF + const bytes = cborg.encode(MAX_SAFE_UINT16); + + const [newOffset, parsedCborUint] = await contract.testParseCBORUint( + bytes, + 0, + ); + + expect(parsedCborUint).eq(MAX_SAFE_UINT16); + expect(newOffset).eq(2 + 1); + }); + + it('Should successfully parse CBOR uint32', async () => { + const MAX_SAFE_UINT32 = 4294967295n; + + // bytes = 0x1AFFFFFFFF + const bytes = cborg.encode(MAX_SAFE_UINT32); + + const [newOffset, parsedCborUint] = await contract.testParseCBORUint( + bytes, + 0, + ); + + expect(parsedCborUint).eq(MAX_SAFE_UINT32); + expect(newOffset).eq(4 + 1); + }); + + it('Should successfully parse CBOR uint64', async () => { + const MAX_SAFE_UINT64 = 18446744073709551615n; + + // bytes = 0x1BFFFFFFFFFFFFFFFF + const bytes = cborg.encode(MAX_SAFE_UINT64); + + const [newOffset, parsedCborUint] = await contract.testParseCBORUint( + bytes, + 0, + ); + + expect(parsedCborUint).eq(MAX_SAFE_UINT64); + expect(newOffset).eq(8 + 1); + }); + + it('Should successfully parse CBOR uint128', async () => { + const MAX_SAFE_UINT128 = 340282366920938463463374607431768211455n; + + const hex = '0xFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFF'; + const uint128bytes = Uint8Array.from( + Buffer.from(hex.replace('0x', ''), 'hex'), + ); + + const bytes = cborg.encode(uint128bytes); + + const [newOffset, parsedCborUint] = await contract.testParseCBORUint( + bytes, + 0, + ); + + expect(parsedCborUint).eq(MAX_SAFE_UINT128); + expect(newOffset).eq(16 + 1); + }); + + it('Should successfully parse CBOR uint256', async () => { + const MAX_SAFE_UINT256 = + 115792089237316195423570985008687907853269984665640564039457584007913129639935n; + + const hex = + '0xFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFF'; + const uint256bytes = Uint8Array.from( + Buffer.from(hex.replace('0x', ''), 'hex'), + ); + + const bytes = cborg.encode(uint256bytes); + + const [newOffset, parsedCborUint] = await contract.testParseCBORUint( + bytes, + 0, + ); + + expect(parsedCborUint).eq(MAX_SAFE_UINT256); + expect(newOffset).eq(33 + 1); + }); + }); });