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: fix CBOR unsigned int decoding in Subcall.sol #325

Merged
merged 9 commits into from
Jul 30, 2024

Conversation

CedarMist
Copy link
Member

fixes #323

@CedarMist CedarMist self-assigned this Jul 1, 2024
@CedarMist CedarMist added contracts Pull requests that update sapphire-contracts solidity labels Jul 1, 2024
Copy link

netlify bot commented Jul 1, 2024

Deploy Preview for oasisprotocol-sapphire-paratime ready!

Name Link
🔨 Latest commit fbca43d
🔍 Latest deploy log https://app.netlify.com/sites/oasisprotocol-sapphire-paratime/deploys/66a88b323494020008a4cb42
😎 Deploy Preview https://deploy-preview-325--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 marked this pull request as ready for review July 1, 2024 13:29
@CedarMist CedarMist requested review from matevz and aefhm July 1, 2024 13:45
@CedarMist
Copy link
Member Author

Ok, minor last thing.

I need to make the subcall stuff not error out if it encounters and unknown key.

contracts/contracts/Subcall.sol Outdated Show resolved Hide resolved
contracts/contracts/Subcall.sol Outdated Show resolved Hide resolved
contracts/contracts/Subcall.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.

FYI this section https://www.rfc-editor.org/rfc/rfc8949.html#name-tag-validity mentions that the 128- and 256-bit integers are encoded as 0x40 (byte arrays).

@matevz
Copy link
Member

matevz commented Jul 2, 2024

FYI this section https://www.rfc-editor.org/rfc/rfc8949.html#name-tag-validity mentions that the 128- and 256-bit integers are encoded as 0x40 (byte arrays).

Having the _cborParseUInt() split into 64-bit and smaller uints and 128-bit and greater uints codepaths opens potential gas attacks? Should we use gas padding to mitigate this?

@CedarMist
Copy link
Member Author

FYI this section https://www.rfc-editor.org/rfc/rfc8949.html#name-tag-validity mentions that the 128- and 256-bit integers are encoded as 0x40 (byte arrays).

Having the _cborParseUInt() split into 64-bit and smaller uints and 128-bit and greater uints codepaths opens potential gas attacks? Should we use gas padding to mitigate this?

I'd prefer if the caller of the receipt parser does the gas padding if it's deemed necessary rather than building it into the parser itself. But yes, I'll add a note in the code about it.

@lubej lubej force-pushed the CedarMist/subcall-cbor-uint-fix branch from 4410fad to 8580d00 Compare July 29, 2024 17:24
@lubej lubej force-pushed the CedarMist/subcall-cbor-uint-fix branch 4 times, most recently from 303cfe7 to 0bb6993 Compare July 30, 2024 06:19
@lubej lubej force-pushed the CedarMist/subcall-cbor-uint-fix branch from 0bb6993 to fbca43d Compare July 30, 2024 06:41
@lubej lubej merged commit 198dfa8 into main Jul 30, 2024
10 checks passed
@lubej lubej deleted the CedarMist/subcall-cbor-uint-fix branch July 30, 2024 08:48
github-actions bot added a commit that referenced this pull request Jul 30, 2024
…edarMist/subcall-cbor-uint-fix

contracts: fix CBOR unsigned int decoding in Subcall.sol 198dfa8
@@ -170,6 +176,11 @@ library Subcall {
(uint64, bytes)
);

// 0xf6 = null, returns null in case receiptId not found
Copy link
Contributor

@lubej lubej Jul 30, 2024

Choose a reason for hiding this comment

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

Also returns null(0xf6) in case epoch is not reached yet.

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 solidity
Projects
None yet
Development

Successfully merging this pull request may close these issues.

_parseCBORUint failing in _decodeReceiptUndelegateStart
3 participants