Skip to content
This repository has been archived by the owner on Sep 30, 2024. It is now read-only.

Clarify webauthn decoding from pr comments #102

Merged
merged 2 commits into from
Oct 2, 2023
Merged

Clarify webauthn decoding from pr comments #102

Changes from 1 commit
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
0441f76
Clarify webauthn decoding from pr comments
jzaki Sep 22, 2023
b2ea1ed
Fix comment value
jzaki Oct 2, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
33 changes: 18 additions & 15 deletions account-integrations/safe/src/SafeWebAuthnPlugin.sol
View file
Open in desktop
Copy link
Contributor

Choose a reason for hiding this comment

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

What did you think about this comment?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I was at a squeeze to get the vars to fit without stack overflow. Adding another call introduces additional var copies and stack usage. I didn't try it for that reason, I can try that as a new PR.

Copy link
Contributor

Choose a reason for hiding this comment

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

Ok makes sense, sounds good to me

Original file line number Diff line number Diff line change
Expand Up @@ -67,7 +67,9 @@ contract SafeWebAuthnPlugin is BaseAccount {
return _publicKey;
}

struct LocalVarStruct {
// Struct declaration to hold multiple local vars.
// Prevents stack from getting too deep for evm.
struct LocalVarWrapper {
bytes1 authenticatorDataFlagMask;
bytes32 clientChallenge;
uint256 clientChallengeDataOffset;
Expand All @@ -81,9 +83,10 @@ contract SafeWebAuthnPlugin is BaseAccount {
bytes calldata clientData;
uint256[2] calldata signature;
uint256[2] calldata pubKey;
LocalVarStruct memory s;
LocalVarWrapper memory wrapper;

{
// scope to contain local variables that can be popped from the stack after use
{
// parse length of all fixed-length params (including length)
uint i = 0;
uint dataLen = 32;
Expand All @@ -92,12 +95,13 @@ contract SafeWebAuthnPlugin is BaseAccount {
i += dataLen; // advance index

// decode fixed length params (values to memory)
dataLen = 4 * 32; //lenFixedParams - 32; // -32 already read length
dataLen = paramLen - 32; // length already read
dataLen -= 2 * 2 * 32; // exclude fixed length arrays
Comment on lines +98 to +99
Copy link
Contributor

Choose a reason for hiding this comment

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

I'm not sure I'm understanding this change, can you explain what's going on here and how this leads to the same value as dataLen = 4 * 32;?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The param length is the length of all fixed bytes including the length parameter (you can add this up from the fixed params or print out to see the value).
The first line subtracts the already-read length, the next line subtract two arrays of length 2 containing 32bytes.

Copy link
Contributor

Choose a reason for hiding this comment

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

As discussed in call, clarified changes and updated one of the comments

(
s.authenticatorDataFlagMask,
wrapper.authenticatorDataFlagMask,
, // some number
s.clientChallenge,
s.clientChallengeDataOffset
wrapper.clientChallenge,
wrapper.clientChallengeDataOffset
) = abi.decode(
userOp.signature[i:i+dataLen],
(
Expand Down Expand Up @@ -130,8 +134,7 @@ contract SafeWebAuthnPlugin is BaseAccount {
paramLen = abi.decode(userOp.signature[i:i+dataLen], (uint256));
i += dataLen; // advance index
// assign authenticatorData to sig splice
dataLen = paramLen;//((paramLen >> 5) + 1) << 5; // (round up to next slot)

dataLen = paramLen;
Copy link
Contributor

Choose a reason for hiding this comment

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

Did you have any thoughts on the question at the end of this comment?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

In that linked suggestion, it moves around how the variables are used throughout. That is:

  • i is always the index at the start of the data,
  • dataLen is always used as the length of data to read
  • paramLen is the length of the parameter

So assigning dataLen to be paramLen is hopefully helpful towards self-documenting code (and if redundant should be optimised out by the compiler anyway).
Using paramLen directly in the slice is different to how the slices are used everywhere else. So even though it would not error, it is less clear as a whole.

Copy link
Contributor

Choose a reason for hiding this comment

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

Thanks for clarifying, that makes sense to me and agree that seems more clear

authenticatorData = userOp.signature[i:i+dataLen];
i += ((dataLen >> 5) + 1) << 5; // advance index (round up to next slot)

Expand All @@ -140,17 +143,17 @@ contract SafeWebAuthnPlugin is BaseAccount {
paramLen = abi.decode(userOp.signature[i:i+dataLen], (uint256));
i += dataLen; // advance index
// assign clientData to sig splice
dataLen = paramLen;// ((paramLen >> 5) + 1) << 5; // (round up to next slot)
dataLen = paramLen;
clientData = userOp.signature[i:i+dataLen];
i += ((dataLen >> 5) + 1) << 5; // advance index (round up to next slot)
}
// i += ((dataLen >> 5) + 1) << 5; // advance index (round up to next slot)
} // end scope to free vars from stack

bool verified = FCL_WebAuthn.checkSignature(
authenticatorData,
s.authenticatorDataFlagMask,
wrapper.authenticatorDataFlagMask,
clientData,
s.clientChallenge,
s.clientChallengeDataOffset,
wrapper.clientChallenge,
wrapper.clientChallengeDataOffset,
signature,
pubKey
);
Expand Down