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

feat: update storage layout of signer #36

Merged
merged 9 commits into from
Aug 13, 2024
Merged
Show file tree
Hide file tree
Changes from 8 commits
Commits
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
98 changes: 47 additions & 51 deletions packages/smart-vaults/src/library/MultiSignerLib.sol
Original file line number Diff line number Diff line change
Expand Up @@ -3,38 +3,31 @@ pragma solidity ^0.8.23;

import { decodeAccountSigner } from "../signers/AccountSigner.sol";
import { decodePasskeySigner } from "../signers/PasskeySigner.sol";
import { Signer } from "../signers/Signer.sol";

/// @notice Storage layout used by this contract.
/// @dev Can allow up to 256 signers.
Copy link
Collaborator

Choose a reason for hiding this comment

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

According to ERC-7201, the annotation @custom:storage-location below this line is ignored when outside of a contract:

Structs with this annotation found outside of contracts are not considered to be namespaces for any contract in the source code.

(Inside the library like it was before it would also be ignored.)

The practical consequence is that tools like OpenZeppelin Upgrades will not consider this struct when analyzing the storage layout of a vault.

A good way to fix this IMO would be to keep the struct here but remove the annotation, and define a new struct specifically for the layout inside the MultiSigner contract.

The names may need to be reviewed. I see this library as another kind of signer so it may belong in src/signers/MultiSigner.sol. As for the contract it could be contract MultiSignerAuth?

contract MultiSignerAuth {
    /// @custom:storage-location erc7201:splits.storage.MultiSigner
    struct MultiSignerAuthStorage {
        MultiSigner signer;
    }

    ...
}

/// @custom:storage-location erc7201:splits.storage.MultiSigner
struct MultiSignerStorage {
/// @dev Number of unique signatures required to validate a message signed by this contract.
uint8 threshold;
/// @dev number of signers
uint8 signerCount;
/// @dev signers of type `Signer`;
mapping(uint8 => Signer) signers;
}

using MultiSignerLib for MultiSignerStorage global;

/**
* @title Multi Signer Library
* @author Splits
*/
library MultiSignerLib {
/* -------------------------------------------------------------------------- */
/* CONSTANTS */
/* -------------------------------------------------------------------------- */

// Size in bytes for an externally owned account (EOA) signer.
uint256 public constant EOA_SIGNER_SIZE = 32;

// Size in bytes for a passkey-based signer.
uint256 public constant PASSKEY_SIGNER_SIZE = 64;

/* -------------------------------------------------------------------------- */
/* STRUCTS */
/* -------------------------------------------------------------------------- */

/// @notice Storage layout used by this contract.
/// @dev Can allow up to 256 signers.
/// @custom:storage-location erc7201:splits.storage.MultiSigner
struct MultiSignerStorage {
/// @dev Number of unique signatures required to validate a message signed by this contract.
uint8 threshold;
/// @dev number of signers
uint8 signerCount;
/// @dev signer bytes;
mapping(uint8 => bytes) signers;
}

struct SignatureWrapper {
/// @dev The index of the signer that signed, see `MultiSigner.signerAtIndex`
uint8 signerIndex;
Expand All @@ -49,19 +42,8 @@ library MultiSignerLib {
/* ERRORS */
/* -------------------------------------------------------------------------- */

/**
* @notice Thrown when a provided signer is neither 64 bytes long (for public key)
* nor a ABI encoded address.
* @param signer The invalid signer.
*/
error InvalidSignerBytesLength(bytes signer);

/**
* @notice Thrown if a provided signer is 32 bytes long but does not fit in an `address` type or if `signer` has
* code.
* @param signer The invalid signer.
*/
error InvalidEthereumAddressOwner(bytes signer);
/// @notice Thrown when Signer is invalid.
error InvalidSigner(Signer signer);

/// @notice Thrown when threshold is greater than number of owners or when zero.
error InvalidThreshold();
Expand All @@ -73,16 +55,37 @@ library MultiSignerLib {
/* FUNCTIONS */
/* -------------------------------------------------------------------------- */

/**
* @notice Sets signer at index in storage.
*
* @dev Throws error when signer is not EOA or Passkey.
*
* @param $_ multi signer storage reference.
* @param signer_ Signer to be set.
* @param index_ Index to set signer at.
*/
function setSigner(MultiSignerStorage storage $_, Signer calldata signer_, uint8 index_) internal {
if (signer_.isPasskey()) {
/// if passkey store signer as is.
$_.signers[index_] = signer_;
} else if (signer_.isEOA()) {
/// if EOA only store slot1 since slot2 is zero.
$_.signers[index_].slot1 = signer_.slot1;
} else {
revert InvalidSigner(signer_);
}
}

/**
* @notice Validates the list of `signers` and `threshold`.
*
* @dev Throws error when number of signers is zero or greater than 255.
* @dev Throws error if `threshold` is zero or greater than number of signers.
*
* @param signers_ abi encoded list of signers (passkey/eoa).
* @param signers_ List of Signer(s).
* @param threshold_ minimum number of signers required for approval.
*/
function validateSigners(bytes[] calldata signers_, uint8 threshold_) internal pure {
function validateSigners(Signer[] calldata signers_, uint8 threshold_) internal pure {
if (signers_.length > 255 || signers_.length == 0) revert InvalidNumberOfSigners();

uint8 numberOfSigners = uint8(signers_.length);
Expand All @@ -97,35 +100,28 @@ library MultiSignerLib {
/**
* @notice Validates the signer.
*
* @dev Throws error when length of signer is neither 32 or 64.
* @dev Throws error if signer is invalid address.
* @dev Throws error when signer is neither an EOA or a passkey.
*/
function validateSigner(bytes memory signer_) internal pure {
if (signer_.length != EOA_SIGNER_SIZE && signer_.length != PASSKEY_SIGNER_SIZE) {
revert InvalidSignerBytesLength(signer_);
}

if (signer_.length == EOA_SIGNER_SIZE) {
if (uint256(bytes32(signer_)) > type(uint160).max) revert InvalidEthereumAddressOwner(signer_);
}
function validateSigner(Signer calldata signer_) internal pure {
if (!signer_.isValid()) revert InvalidSigner(signer_);
}

/**
* @notice validates if the signature provided by the signer at `signerIndex` is valid for the hash.
*/
function isValidSignature(
bytes32 hash_,
bytes memory signer_,
Signer memory signer_,
bytes memory signature_
)
internal
view
returns (bool isValid)
{
if (signer_.length == EOA_SIGNER_SIZE) {
isValid = decodeAccountSigner(signer_).isValidSignature(hash_, signature_);
} else if (signer_.length == PASSKEY_SIGNER_SIZE) {
if (signer_.isPasskeyMem()) {
isValid = decodePasskeySigner(signer_).isValidSignature(hash_, signature_);
} else {
isValid = decodeAccountSigner(signer_).isValidSignature(hash_, signature_);
}
}

Expand Down
5 changes: 3 additions & 2 deletions packages/smart-vaults/src/signers/AccountSigner.sol
Original file line number Diff line number Diff line change
@@ -1,15 +1,16 @@
// SPDX-License-Identifier: GPL-3.0-or-later
pragma solidity ^0.8.23;

import { Signer } from "./Signer.sol";
import { SignatureCheckerLib } from "solady/utils/SignatureCheckerLib.sol";

/**
* @notice A signer backed by an EOA or ERC-1271 smart account.
*/
type AccountSigner is address;

function decodeAccountSigner(bytes memory signer) pure returns (AccountSigner) {
return AccountSigner.wrap(abi.decode(signer, (address)));
function decodeAccountSigner(Signer memory signer_) pure returns (AccountSigner) {
return AccountSigner.wrap(address(uint160(uint256(signer_.slot1))));
}

using AccountSignerLib for AccountSigner global;
Expand Down
5 changes: 3 additions & 2 deletions packages/smart-vaults/src/signers/PasskeySigner.sol
Original file line number Diff line number Diff line change
@@ -1,6 +1,7 @@
// SPDX-License-Identifier: GPL-3.0-or-later
pragma solidity ^0.8.23;

import { Signer } from "./Signer.sol";
import { WebAuthn } from "@web-authn/WebAuthn.sol";

/**
Expand All @@ -11,8 +12,8 @@ struct PasskeySigner {
uint256 y;
}

function decodePasskeySigner(bytes memory signer) pure returns (PasskeySigner memory) {
return abi.decode(signer, (PasskeySigner));
function decodePasskeySigner(Signer memory signer_) pure returns (PasskeySigner memory) {
return PasskeySigner(uint256(signer_.slot1), uint256(signer_.slot2));
}

using PasskeySignerLib for PasskeySigner global;
Expand Down
61 changes: 61 additions & 0 deletions packages/smart-vaults/src/signers/Signer.sol
Original file line number Diff line number Diff line change
@@ -0,0 +1,61 @@
// SPDX-License-Identifier: GPL-3.0-or-later
pragma solidity ^0.8.23;

/**
* @notice An EOA or Passkey signer.
*
* @dev For a Signer to be valid it has to be either an EOA or a Passkey signer.
* - EOA -> slot2 has to be empty and slot1 has to be a valid address.
* - Passkey -> both slot1 and slot2 have to be non empty.
*/
struct Signer {
r0ohafza marked this conversation as resolved.
Show resolved Hide resolved
bytes32 slot1;
bytes32 slot2;
}

function createSigner(address signer_) pure returns (Signer memory) {
return Signer(bytes32(uint256(uint160(signer_))), bytes32(0));
}

function createSigner(uint256 x_, uint256 y_) pure returns (Signer memory) {
return Signer(bytes32(x_), bytes32(y_));
}

using SignerLib for Signer global;

library SignerLib {
/* -------------------------------------------------------------------------- */
/* CONSTANTS */
/* -------------------------------------------------------------------------- */

bytes32 constant ZERO = bytes32(0);

/* -------------------------------------------------------------------------- */
/* FUNCTIONS */
/* -------------------------------------------------------------------------- */

function isEOA(Signer calldata signer_) internal pure returns (bool) {
uint256 slot1 = uint256(bytes32(signer_.slot1));
return signer_.slot2 == ZERO && slot1 <= type(uint160).max && slot1 > 0;
}

function isPasskey(Signer calldata signer_) internal pure returns (bool) {
Copy link
Collaborator

Choose a reason for hiding this comment

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

According to this Stack Exchange answer the x coordinate of a public key could technically be zero. Based on that, I would only require slot2 to be non-zero. Not my area of expertise though, so it would be great if someone else could confirm this understanding.

return signer_.slot1 != ZERO && signer_.slot2 != ZERO;
}

function isPasskeyMem(Signer memory signer_) internal pure returns (bool) {
return signer_.slot1 != ZERO && signer_.slot2 != ZERO;
}

function isValid(Signer calldata signer_) internal pure returns (bool) {
r0ohafza marked this conversation as resolved.
Show resolved Hide resolved
return isEOA(signer_) || isPasskey(signer_);
}

function isEmpty(Signer calldata signer_) internal pure returns (bool) {
return signer_.slot1 == ZERO && signer_.slot2 == ZERO;
}

function isEmptyMem(Signer memory signer_) internal pure returns (bool) {
return signer_.slot1 == ZERO && signer_.slot2 == ZERO;
}
}
Loading