contracts: fix CBOR unsigned int decoding in Subcall.sol

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

contracts/contracts/Subcall.sol

@@ -52,7 +52,7 @@ library Subcall {
     error InvalidMap();
 
     /// While parsing CBOR structure, data length was unexpected
-    error InvalidLength();
+    error InvalidLength(uint256);
 
     /// 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();
 
+    /// Value 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.*`.
@@ -170,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));
         }
@@ -182,11 +193,42 @@ library Subcall {
         pure
         returns (uint256 newOffset, uint256 value)
     {
-        if (result[offset] & 0x40 != 0x40) revert InvalidLength();
+        uint8 prefix = uint8(result[offset]);
+        uint256 len;
 
-        uint256 len = uint8(result[offset++]) ^ 0x40;
+        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;
+        }
+        // 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))
@@ -267,9 +309,6 @@ library Subcall {
                 (offset, endReceipt) = _parseCBORUint64(result, offset);
 
                 hasReceipt = true;
-            } else {
-                // TODO: skip unknown keys & values? For forward compatibility
-                revert InvalidKey();
             }
         }
 
@@ -296,9 +335,6 @@ library Subcall {
                 (offset, amount) = _parseCBORUint128(result, offset);
 
                 hasAmount = true;
-            } else {
-                // TODO: skip unknown keys & values? For forward compatibility
-                revert InvalidKey();
             }
         }
 
@@ -321,7 +357,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]);

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);
+    }
 }

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",

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) {
@@ -104,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;
@@ -240,8 +248,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 +290,29 @@ 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'),
     );
   });
 
@@ -350,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);
+    });
+  });
 });