Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

contracts: implement calldata encryption in Solidity #436

Closed
wants to merge 7 commits into from

Conversation

CedarMist
Copy link
Member

@CedarMist CedarMist commented Oct 11, 2024

Fixes: #428

  • HMAC SHA512-256 (but discovered that was unnecessary. Included anyway as it may be useful.)
  • Introduces subcall_static to perform subcall with staticcall, makes several Subcall methods view
  • Split CBOR stuff into its own file + tests.
  • Allow passing manual nonce to Cipher for encryption (useful for tests & reproducibility)
  • Various changes to clean up solhint warnings
  • Transaction calldata encryption + tests

Re: CBOR, I split the CBOR related stuff into individual functions, as otherwise you start needing to deploy the whole CBOR library on its own an link it into a library, whereas using individual function they get pulled in as & when necessary.

@CedarMist CedarMist added the contracts Pull requests that update sapphire-contracts label Oct 11, 2024
@CedarMist CedarMist self-assigned this Oct 11, 2024
Copy link

netlify bot commented Oct 11, 2024

Deploy Preview for oasisprotocol-sapphire-paratime ready!

Name Link
🔨 Latest commit 5482d3d
🔍 Latest deploy log https://app.netlify.com/sites/oasisprotocol-sapphire-paratime/deploys/670df93b6f63b000088114ca
😎 Deploy Preview https://deploy-preview-436--oasisprotocol-sapphire-paratime.netlify.app
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site configuration.

@CedarMist CedarMist added this to the contracts-0.3.0 milestone Oct 12, 2024
@CedarMist CedarMist marked this pull request as ready for review October 12, 2024 15:39
@CedarMist CedarMist requested review from matevz, aefhm and rube-de October 12, 2024 15:39
);

return
abi.encodePacked(
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I suspect the output bytecode for this is going to be huge

expect(parsedCborUint).eq(MAX_SAFE_UINT128);
expect(newOffset).eq(16 + 1);

//expect(await contract.testUintRoundtrip(MAX_SAFE_UINT128)).eq(true);
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

CBOR encoding integers larger than 64bit seems to vary between libraries, some use tagging, others don't.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not a blocker right. I'm sure we'll get dev feedback if this is buggy?

contracts/.prettierignore Outdated Show resolved Hide resolved
clients/js/src/cipher.ts Show resolved Hide resolved
contracts/contracts/CalldataEncryption.sol Show resolved Hide resolved
contracts/contracts/tests/TestCBOR.sol Outdated Show resolved Hide resolved
expect(parsedCborUint).eq(MAX_SAFE_UINT128);
expect(newOffset).eq(16 + 1);

//expect(await contract.testUintRoundtrip(MAX_SAFE_UINT128)).eq(true);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not a blocker right. I'm sure we'll get dev feedback if this is buggy?

contracts/test/calldata.spec.ts Show resolved Hide resolved
contracts/contracts/Subcall.sol Outdated Show resolved Hide resolved
contracts/contracts/CBOR.sol Outdated Show resolved Hide resolved
contracts/contracts/CalldataEncryption.sol Outdated Show resolved Hide resolved
Copy link
Member

@matevz matevz left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I know I'm PITA, but this is tedious to review. Why not make life easier and split this PR into five:

  • CBOR.sol
  • actual calldata encryption PR
  • OPL support for encrypted calldata
  • HMAC_sha512_256
  • 0.3.0 version bump (changelog & new release?)

if (len > 0x20) revert CBOR_Error_InvalidLength(len);

assembly {
value := mload(add(add(0x20, result), offset))
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Huh?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This loads a 256bit word from an offset of a byte array, the first 256bits of the byte array is the length prefix, which is skipped.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah, I see. Maybe add a comment?

Suggested change
value := mload(add(add(0x20, result), offset))
// Load value from the result array at position 32+offset.
value := mload(add(add(0x20, result), offset))

@CedarMist
Copy link
Member Author

Unfortunately splitting a small PR into lots of even smaller minor PRs is also very tedious. I can re-commit them as individual commits within the PR (that would be slightly less tedious)?

);
}

function encryptCallData(bytes memory in_data)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please document this and add a short example similar to other functions in Sapphire.sol.

// OPAD ^ IPAD, (OPAD = 0x5c)
bytes32 constant HMAC_OPAD_XOR_IPAD = 0x6a6a6a6a6a6a6a6a6a6a6a6a6a6a6a6a6a6a6a6a6a6a6a6a6a6a6a6a6a6a6a6a;

function HMAC_sha512_256(bytes memory key, bytes memory message)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please document this and add a short example similar to other functions in Sapphire.sol.

Comment on lines +11 to 13
constructor() payable { // solhint-disable-line
// Do nothing, but allow balances to be sent on construction.
}
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Does this work? Same below.

Suggested change
constructor() payable { // solhint-disable-line
// Do nothing, but allow balances to be sent on construction.
}
// Do nothing, but allow balances to be sent on construction.
constructor() payable {}

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
contracts Pull requests that update sapphire-contracts
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Support generating encrypted transactions on-chain
4 participants