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.

Sign up for GitHub

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

fix: clean up multi signer auth #40

Merged
merged 3 commits into from
Aug 14, 2024
Merged

fix: clean up multi signer auth #40

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
0d526ac
fix: clean up multi signer auth
r0ohafza Aug 14, 2024
402e05b
feat: mapping to array
r0ohafza Aug 14, 2024
c90353e
fix: address comment
r0ohafza Aug 14, 2024
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
104 changes: 86 additions & 18 deletions packages/smart-vaults/src/signers/MultiSigner.sol
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -14,7 +14,7 @@ struct MultiSigner {
/// @dev number of signers
uint8 signerCount;
/// @dev signers of type `Signer`;
mapping(uint8 => Signer) signers;
Signer[256] signers;
}

using MultiSignerLib for MultiSigner global;
Expand Down Expand Up @@ -52,6 +52,18 @@ library MultiSignerLib {
/// @notice Thrown when number of signers is more than 256.
error InvalidNumberOfSigners();

/**
* @notice Thrown when trying to remove an empty signer.
* @param index Index of the empty signer.
*/
error SignerNotPresent(uint8 index);

/**
* @notice Thrown when trying to replace an existing signer.
* @param index Index of the existing signer.
*/
error SignerAlreadyPresent(uint8 index);

/* -------------------------------------------------------------------------- */
/* FUNCTIONS */
/* -------------------------------------------------------------------------- */
Expand All @@ -74,54 +86,106 @@ library MultiSignerLib {
/**
* @notice Adds signer at index in storage.
*
* @dev Throws error when a signer is already present at index.
* @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. Assumed to be currently unoccupied.
* @param index_ Index to set signer at.
*/
function addSigner(MultiSigner 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_);
}
if (!$_.signers[index_].isEmptyMem()) revert SignerAlreadyPresent(index_);

_addSigner($_, signer_, index_);

$_.signerCount = $_.signerCount + 1;
}

/**
* @notice Removes signer at the given `index`.
*
* @dev Reverts if 'threshold' is equal to signer count.
* @dev Reverts if signer is empty at `index`.
*
* @param $_ Multi signer storage reference.
* @param index_ The index of the signer to be removed.
* @return signer Signer being removed.
*/
function removeSigner(MultiSigner storage $_, uint8 index_) internal {
function removeSigner(MultiSigner storage $_, uint8 index_) internal returns (Signer memory signer) {
uint8 signerCount = $_.signerCount;

if (signerCount == $_.threshold) revert InvalidThreshold();

signer = $_.signers[index_];

if (signer.isEmptyMem()) revert SignerNotPresent(index_);

delete $_.signers[index_];
$_.signerCount = signerCount - 1;
}

/**
* @notice Updates threshold of the signer set.
*
* @dev Reverts if 'threshold' is greater than signer count.
* @dev Reverts if 'threshold' is 0.
*
* @param $_ Multi signer storage reference.
* @param threshold_ The new signer set threshold.
*/
function updateThreshold(MultiSigner storage $_, uint8 threshold_) internal {
if (threshold_ == 0) revert InvalidThreshold();

if ($_.signerCount < threshold_) revert InvalidThreshold();

$_.threshold = threshold_;
}

/**
* @notice Updates signer count of the signer set.
* @notice Initialize the signers.
*
* @param $_ Multi signer storage reference.
* @param signerCount_ The new signer count.
* @dev Intended to be called when initializing the signer set.
* @dev Reverts if signer is neither an EOA or a passkey.
* @dev Reverts if 'threshold' is less than number of signers.
* @dev Reverts if 'threshold' is 0.
* @dev Reverts if number of signers is more than 256.
*
* @param signers_ The initial set of signers.
* @param threshold_ The number of signers needed for approval.
*/
function updateSignerCount(MultiSigner storage $_, uint8 signerCount_) internal {
$_.signerCount = signerCount_;
function initializeSigners(MultiSigner storage $_, Signer[] calldata signers_, uint8 threshold_) internal {
if (signers_.length > type(uint8).max) revert InvalidNumberOfSigners();

uint8 numSigners = uint8(signers_.length);

if (numSigners < threshold_ || threshold_ < 1) revert InvalidThreshold();

for (uint8 i; i < numSigners; i++) {
_addSigner($_, signers_[i], i);
}

$_.signerCount = numSigners;
$_.threshold = threshold_;
}

/// @dev reverts if signer is neither an EOA or a Passkey.
/// @dev replaces signer at `index`. Should be used with proper checks.
function _addSigner(MultiSigner storage $_, Signer calldata signer_, uint8 index_) private {
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_);
}
}

/* -------------------------------------------------------------------------- */
/* VALIDATE SIGNERS */
/* -------------------------------------------------------------------------- */

/**
* @notice Validates the list of `signers` and `threshold`.
*
Expand All @@ -132,7 +196,7 @@ library MultiSignerLib {
* @param threshold_ minimum number of signers required for approval.
*/
function validateSigners(Signer[] calldata signers_, uint8 threshold_) internal pure {
if (signers_.length > type(uint8).max || signers_.length == 0) revert InvalidNumberOfSigners();
if (signers_.length > type(uint8).max) revert InvalidNumberOfSigners();

uint8 numberOfSigners = uint8(signers_.length);

Expand All @@ -152,6 +216,10 @@ library MultiSignerLib {
if (!signer_.isValid()) revert InvalidSigner(signer_);
}

/* -------------------------------------------------------------------------- */
/* VALIDATE SIGNATURES */
/* -------------------------------------------------------------------------- */

/**
* @notice validates if `hash_` was signed by the signer set present in `$_`
*
Expand Down
93 changes: 26 additions & 67 deletions packages/smart-vaults/src/utils/MultiSignerAuth.sol
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -35,30 +35,15 @@ abstract contract MultiSignerAuth {
}

/* -------------------------------------------------------------------------- */
/* ERRORS */
/* EVENTS */
/* -------------------------------------------------------------------------- */

/// @notice Thrown when threshold is greater than number of signers or when zero.
error InvalidThreshold();

/// @notice Thrown when number of signers is more than 256.
error InvalidNumberOfSigners();

/**
* @notice Thrown when trying to remove an empty signer.
* @param index Index of the empty signer.
*/
error SignerNotPresent(uint8 index);

/**
* @notice Thrown when trying to replace an existing signer.
* @param index Index of the existing signer.
* @notice Emitted when MultiSigner is initialized.
* @param signers Initial set of signers.
* @param threshold Initial threshold for the signer set.
*/
error SignerAlreadyPresent(uint8 index);

/* -------------------------------------------------------------------------- */
/* EVENTS */
/* -------------------------------------------------------------------------- */
event InitializedSigners(Signer[] signers, uint8 threshold);

/**
* @notice Emitted when a new signer is registered.
Expand Down Expand Up @@ -93,19 +78,19 @@ abstract contract MultiSignerAuth {
/* PUBLIC VIEW FUNCTIONS */
/* -------------------------------------------------------------------------- */

/// @notice Returns the owner bytes at the given `index`.
function getSigner(uint8 index_) public view virtual returns (Signer memory) {
return _getMultiSignerAuthStorage().signers.getSigner(index_);
/// @notice Returns the Signer at the given `index`.
function getSigner(uint8 index_) public view returns (Signer memory) {
return _getMultiSignerStorage().getSigner(index_);
}

/// @notice Returns the current number of signers
function getSignerCount() public view virtual returns (uint8) {
return _getMultiSignerAuthStorage().signers.getSignerCount();
function getSignerCount() public view returns (uint8) {
return _getMultiSignerStorage().getSignerCount();
}

/// @notice Returns the threshold
function getThreshold() public view virtual returns (uint8) {
return _getMultiSignerAuthStorage().signers.getThreshold();
function getThreshold() public view returns (uint8) {
return _getMultiSignerStorage().getThreshold();
}

/* -------------------------------------------------------------------------- */
Expand All @@ -115,18 +100,16 @@ abstract contract MultiSignerAuth {
/**
* @notice Adds a `signer` at the `index`.
*
* @dev Throws error when a signer is already present at index.
* @dev Reverts if Signer is neither EOA or Passkey.
*
* @param signer_ The owner raw bytes to register.
* @param index_ The index to register the signer.
*/
function addSigner(Signer calldata signer_, uint8 index_) external onlyAuthorized {
MultiSignerAuthStorage storage $ = _getMultiSignerAuthStorage();

if (!$.signers.getSigner(index_).isEmptyMem()) revert SignerAlreadyPresent(index_);
MultiSigner storage $ = _getMultiSignerStorage();
wminshew marked this conversation as resolved.
Show resolved Hide resolved

$.signers.addSigner(signer_, index_);
$.signers.updateSignerCount($.signers.getSignerCount() + 1);
$.addSigner(signer_, index_);

emit AddSigner(index_, signer_);
}
Expand All @@ -140,18 +123,9 @@ abstract contract MultiSignerAuth {
* @param index_ The index of the signer to be removed.
*/
function removeSigner(uint8 index_) external onlyAuthorized {
MultiSignerAuthStorage storage $ = _getMultiSignerAuthStorage();

uint8 signerCount = $.signers.getSignerCount();
MultiSigner storage $ = _getMultiSignerStorage();

if (signerCount == $.signers.getThreshold()) revert InvalidThreshold();

Signer memory signer = $.signers.getSigner(index_);

if (signer.isEmptyMem()) revert SignerNotPresent(index_);

$.signers.removeSigner(index_);
$.signers.updateSignerCount(signerCount - 1);
Signer memory signer = $.removeSigner(index_);

emit RemoveSigner(index_, signer);
}
Expand All @@ -165,12 +139,9 @@ abstract contract MultiSignerAuth {
* @param threshold_ The new signer set threshold.
*/
function updateThreshold(uint8 threshold_) external onlyAuthorized {
if (threshold_ == 0) revert InvalidThreshold();
MultiSigner storage $ = _getMultiSignerStorage();

MultiSignerAuthStorage storage $ = _getMultiSignerAuthStorage();
if ($.signers.getSignerCount() < threshold_) revert InvalidThreshold();

$.signers.updateThreshold(threshold_);
$.updateThreshold(threshold_);

emit UpdateThreshold(threshold_);
}
Expand All @@ -182,10 +153,12 @@ abstract contract MultiSignerAuth {
function _authorize() internal virtual;

/// @notice Helper function to get storage reference to the `MultiSignerStorage` struct.
function _getMultiSignerAuthStorage() internal pure returns (MultiSignerAuthStorage storage $) {
function _getMultiSignerStorage() internal view returns (MultiSigner storage) {
MultiSignerAuthStorage storage $;
assembly ("memory-safe") {
$.slot := _MUTLI_SIGNER_AUTH_STORAGE_LOCATION
}
return $.signers;
}

/**
Expand All @@ -200,24 +173,10 @@ abstract contract MultiSignerAuth {
* @param signers_ The initial set of signers.
* @param threshold_ The number of signers needed for approval.
*/
function _initializeSigners(Signer[] calldata signers_, uint8 threshold_) internal virtual {
if (signers_.length > type(uint8).max || signers_.length == 0) revert InvalidNumberOfSigners();

uint8 numSigners = uint8(signers_.length);

if (numSigners < threshold_ || threshold_ < 1) revert InvalidThreshold();
function _initializeMultiSignerAuth(Signer[] calldata signers_, uint8 threshold_) internal {
MultiSigner storage $ = _getMultiSignerStorage();
$.initializeSigners(signers_, threshold_);

MultiSignerAuthStorage storage $ = _getMultiSignerAuthStorage();

for (uint8 i; i < numSigners; i++) {
$.signers.addSigner(signers_[i], i);

emit AddSigner(i, signers_[i]);
}

$.signers.updateSignerCount(numSigners);
$.signers.updateThreshold(threshold_);

emit UpdateThreshold(threshold_);
emit InitializedSigners(signers_, threshold_);
}
}
10 changes: 3 additions & 7 deletions packages/smart-vaults/src/vault/SmartVault.sol
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -168,7 +168,7 @@ contract SmartVault is IAccount, Ownable, UUPSUpgradeable, MultiSignerAuth, ERC1
if (msg.sender != FACTORY) revert OnlyFactory();

_initializeOwner(owner_);
_initializeSigners(signers_, threshold_);
_initializeMultiSignerAuth(signers_, threshold_);
}

/**
Expand Down Expand Up @@ -313,9 +313,7 @@ contract SmartVault is IAccount, Ownable, UUPSUpgradeable, MultiSignerAuth, ERC1

/// @dev validates if the given hash (ERC1271) was signed by the signers.
function _isValidSignature(bytes32 hash_, bytes calldata signature_) internal view override returns (bool) {
return _getMultiSignerAuthStorage().signers.isValidSignature(
hash_, abi.decode(signature_, (ERC1271Signature)).signatures
);
return _getMultiSignerStorage().isValidSignature(hash_, abi.decode(signature_, (ERC1271Signature)).signatures);
}

function _validateSingleUserOp(
Expand Down Expand Up @@ -361,9 +359,7 @@ contract SmartVault is IAccount, Ownable, UUPSUpgradeable, MultiSignerAuth, ERC1
view
returns (uint256 validationData)
{
MultiSignerAuthStorage storage $ = _getMultiSignerAuthStorage();

return $.signers.isValidSignature(lightHash_, hash_, signatures)
return _getMultiSignerStorage().isValidSignature(lightHash_, hash_, signatures)
? UserOperationLib.VALID_SIGNATURE
: UserOperationLib.INVALID_SIGNATURE;
}
Expand Down
6 changes: 2 additions & 4 deletions packages/smart-vaults/test/MultiSigner.t.sol
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -47,9 +47,7 @@ contract MultiSignerTest is BaseTest {

function test_initialize_signers() public {
vm.expectEmit();
emit MultiSignerAuth.AddSigner(0, createSigner(ALICE.addr));
emit MultiSignerAuth.AddSigner(1, createSigner(BOB.addr));
emit MultiSignerAuth.AddSigner(2, createSigner(MIKE.x, MIKE.y));
emit MultiSignerAuth.InitializedSigners(signers, 1);
initializeSigners();

assertEq(multiSigner.getSigner(0), createSigner(ALICE.addr));
Expand All @@ -70,7 +68,7 @@ contract MultiSignerTest is BaseTest {
}

function test_initialize_signers_RevertWhen_zeroSigners() public {
vm.expectRevert(InvalidNumberOfSigners.selector);
vm.expectRevert(InvalidThreshold.selector);
multiSigner.initialize(ALICE.addr, new Signer[](0), 1);
}

Expand Down
2 changes: 1 addition & 1 deletion packages/smart-vaults/test/SmartVault.t.sol
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -148,7 +148,7 @@ contract SmartVaultTest is BaseTest {
}

function test_initialize_RevertsWhen_signersIsZero() public {
vm.expectRevert(abi.encodeWithSelector(InvalidNumberOfSigners.selector));
vm.expectRevert(abi.encodeWithSelector(InvalidThreshold.selector));
vm.prank(address(smartVaultFactory));
vault.initialize(root, new Signer[](0), 1);
}
Expand Down
Loading