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

Commit

Permalink
bySig fixes
Browse files Browse the repository at this point in the history
  • Loading branch information
@0xrajath
0xrajath committed Dec 8, 2023
1 parent d1a1375 commit 797689c
Show file tree
Hide file tree
Showing 5 changed files with 117 additions and 255 deletions.
21 changes: 10 additions & 11 deletions src/token-voting/TokenholderActionCreator.sol
View file
Original file line number Diff line number Diff line change
Expand Up @@ -27,25 +27,25 @@ abstract contract TokenholderActionCreator is Initializable {
/// @dev This role is expected to have the permissions to create appropriate actions.
uint8 public role;

/// @dev EIP-712 actionInfo typehash.
bytes32 internal constant ACTION_INFO_TYPEHASH = keccak256(
"ActionInfo(uint256 id,address creator,uint8 creatorRole,address strategy,address target,uint256 value,bytes data)"
/// @dev EIP-712 base typehash.
bytes32 internal constant EIP712_DOMAIN_TYPEHASH =
keccak256("EIP712Domain(string name,string version,uint256 chainId,address verifyingContract)");

/// @dev EIP-712 createAction typehash.
bytes32 internal constant CREATE_ACTION_TYPEHASH = keccak256(
"CreateAction(address tokenHolder,address strategy,address target,uint256 value,bytes data,string description,uint256 nonce)"
);

/// @dev EIP-712 cancelAction typehash.
bytes32 internal constant CANCEL_ACTION_TYPEHASH = keccak256(
"CancelAction(address tokenHolder,ActionInfo actionInfo,uint256 nonce)ActionInfo(uint256 id,address creator,uint8 creatorRole,address strategy,address target,uint256 value,bytes data)"
);

/// @dev EIP-712 createAction typehash.
bytes32 internal constant CREATE_ACTION_TYPEHASH = keccak256(
"CreateAction(address tokenHolder,uint8 role,address strategy,address target,uint256 value,bytes data,string description,uint256 nonce)"
/// @dev EIP-712 actionInfo typehash.
bytes32 internal constant ACTION_INFO_TYPEHASH = keccak256(
"ActionInfo(uint256 id,address creator,uint8 creatorRole,address strategy,address target,uint256 value,bytes data)"
);

/// @dev EIP-712 base typehash.
bytes32 internal constant EIP712_DOMAIN_TYPEHASH =
keccak256("EIP712Domain(string name,string version,uint256 chainId,address verifyingContract)");

/// @notice The address of the tokenholder that created the action.
mapping(uint256 => address) public actionCreators;

Expand Down Expand Up @@ -283,7 +283,6 @@ abstract contract TokenholderActionCreator is Initializable {
abi.encode(
CREATE_ACTION_TYPEHASH,
tokenHolder,
role,
address(strategy),
target,
value,
Expand Down
125 changes: 44 additions & 81 deletions test/token-voting/ERC20TokenholderActionCreator.t.sol
View file
Original file line number Diff line number Diff line change
Expand Up @@ -32,7 +32,6 @@ contract ERC20TokenholderActionCreatorTest is LlamaTokenVotingTestSetup, LlamaCo
event ActionThresholdSet(uint256 newThreshold);

ERC20TokenholderActionCreator erc20TokenholderActionCreator;
uint8 erc20TokenholderActionCreatorRole;

function setUp() public virtual override {
LlamaTokenVotingTestSetup.setUp();
Expand All @@ -48,7 +47,7 @@ contract ERC20TokenholderActionCreatorTest is LlamaTokenVotingTestSetup, LlamaCo
mineBlock();

// Deploy ERC20 Token Voting Module.
(erc20TokenholderActionCreator,) = _deployERC20TokenVotingModule();
(erc20TokenholderActionCreator,) = _deployERC20TokenVotingModuleAndSetRole();

// Setting ERC20TokenHolderActionCreator's EIP-712 Domain Hash
setDomainHash(
Expand All @@ -61,19 +60,11 @@ contract ERC20TokenholderActionCreatorTest is LlamaTokenVotingTestSetup, LlamaCo
);
}

function _initRoleSetRoleHolderSetRolePermissionToTokenholderActionCreator() internal {
// Init role, assign policy, and assign permission for `setRoleHolder` to the TokenholderActionCreator.
vm.startPrank(address(EXECUTOR));
POLICY.initializeRole(TOKEN_VOTING_ACTION_CREATOR_ROLE_DESC);
erc20TokenholderActionCreatorRole = POLICY.numRoles();
POLICY.setRoleHolder(
erc20TokenholderActionCreatorRole,
address(erc20TokenholderActionCreator),
DEFAULT_ROLE_QTY,
DEFAULT_ROLE_EXPIRATION
);
function _setRolePermissionToTokenholderActionCreator() internal {
// Assign permission for `MockProtocol.pause` to the TokenholderActionCreator.
vm.prank(address(EXECUTOR));
POLICY.setRolePermission(
erc20TokenholderActionCreatorRole,
tokenVotingActionCreatorRole,
ILlamaPolicy.PermissionData(address(mockProtocol), PAUSE_SELECTOR, address(STRATEGY)),
true
);
Expand Down Expand Up @@ -131,9 +122,7 @@ contract CreateAction is ERC20TokenholderActionCreatorTest {

vm.expectRevert(abi.encodeWithSelector(TokenholderActionCreator.InsufficientBalance.selector, 0));
vm.prank(notTokenHolder);
erc20TokenholderActionCreator.createAction(
erc20TokenholderActionCreatorRole, STRATEGY, address(mockProtocol), 0, data, ""
);
erc20TokenholderActionCreator.createAction(STRATEGY, address(mockProtocol), 0, data, "");
}

function test_RevertsIf_TokenholderActionCreatorDoesNotHavePermission() public {
Expand All @@ -145,14 +134,12 @@ contract CreateAction is ERC20TokenholderActionCreatorTest {

vm.expectRevert(ILlamaCore.PolicyholderDoesNotHavePermission.selector);
vm.prank(tokenHolder1);
erc20TokenholderActionCreator.createAction(
erc20TokenholderActionCreatorRole, STRATEGY, address(mockProtocol), 0, data, ""
);
erc20TokenholderActionCreator.createAction(STRATEGY, address(mockProtocol), 0, data, "");
}

function test_ProperlyCreatesAction() public {
// Assigns Policy to TokenholderActionCreator.
_initRoleSetRoleHolderSetRolePermissionToTokenholderActionCreator();
// Assigns Permission to TokenholderActionCreator.
_setRolePermissionToTokenholderActionCreator();

// Mint tokens to tokenholder so that they can create action.
erc20VotesToken.mint(tokenHolder1, ERC20_CREATION_THRESHOLD);
Expand All @@ -166,19 +153,10 @@ contract CreateAction is ERC20TokenholderActionCreatorTest {

vm.expectEmit();
emit ActionCreated(
actionCount,
address(tokenHolder1),
erc20TokenholderActionCreatorRole,
STRATEGY,
address(mockProtocol),
0,
data,
""
actionCount, address(tokenHolder1), tokenVotingActionCreatorRole, STRATEGY, address(mockProtocol), 0, data, ""
);
vm.prank(tokenHolder1);
uint256 actionId = erc20TokenholderActionCreator.createAction(
erc20TokenholderActionCreatorRole, STRATEGY, address(mockProtocol), 0, data, ""
);
uint256 actionId = erc20TokenholderActionCreator.createAction(STRATEGY, address(mockProtocol), 0, data, "");

Action memory action = CORE.getAction(actionId);
assertEq(actionId, actionCount);
Expand All @@ -190,8 +168,8 @@ contract CreateActionBySig is ERC20TokenholderActionCreatorTest {
function setUp() public virtual override {
ERC20TokenholderActionCreatorTest.setUp();

// Assigns Policy to TokenholderActionCreator.
_initRoleSetRoleHolderSetRolePermissionToTokenholderActionCreator();
// Assigns Permission to TokenholderActionCreator.
_setRolePermissionToTokenholderActionCreator();

// Mint tokens to tokenholder so that they can create action.
erc20VotesToken.mint(tokenHolder1, ERC20_CREATION_THRESHOLD);
Expand All @@ -211,32 +189,22 @@ contract CreateActionBySig is ERC20TokenholderActionCreatorTest {
view
returns (uint8 v, bytes32 r, bytes32 s)
{
LlamaCoreSigUtils.CreateActionBySig memory _createAction = LlamaCoreSigUtils.CreateActionBySig({
role: erc20TokenholderActionCreatorRole,
LlamaCoreSigUtils.CreateAction memory _createAction = LlamaCoreSigUtils.CreateAction({
tokenHolder: tokenHolder1,
strategy: address(STRATEGY),
target: address(mockProtocol),
value: 0,
data: abi.encodeCall(mockProtocol.pause, (true)),
description: description,
tokenHolder: tokenHolder1,
nonce: 0
});
bytes32 digest = getCreateActionBySigTypedDataHash(_createAction);
bytes32 digest = getCreateActionTypedDataHash(_createAction);
(v, r, s) = vm.sign(privateKey, digest);
}

function createActionBySig(uint8 v, bytes32 r, bytes32 s) internal returns (uint256 actionId) {
actionId = erc20TokenholderActionCreator.createActionBySig(
tokenHolder1,
erc20TokenholderActionCreatorRole,
STRATEGY,
address(mockProtocol),
0,
abi.encodeCall(mockProtocol.pause, (true)),
"",
v,
r,
s
tokenHolder1, STRATEGY, address(mockProtocol), 0, abi.encodeCall(mockProtocol.pause, (true)), "", v, r, s
);
}

Expand All @@ -248,7 +216,7 @@ contract CreateActionBySig is ERC20TokenholderActionCreatorTest {

vm.expectEmit();
emit ActionCreated(
actionCount, tokenHolder1, erc20TokenholderActionCreatorRole, STRATEGY, address(mockProtocol), 0, data, ""
actionCount, tokenHolder1, tokenVotingActionCreatorRole, STRATEGY, address(mockProtocol), 0, data, ""
);

uint256 actionId = createActionBySig(v, r, s);
Expand All @@ -270,7 +238,7 @@ contract CreateActionBySig is ERC20TokenholderActionCreatorTest {
emit ActionCreated(
actionCount,
tokenHolder1,
erc20TokenholderActionCreatorRole,
tokenVotingActionCreatorRole,
STRATEGY,
address(mockProtocol),
0,
Expand All @@ -280,7 +248,6 @@ contract CreateActionBySig is ERC20TokenholderActionCreatorTest {

uint256 actionId = erc20TokenholderActionCreator.createActionBySig(
tokenHolder1,
erc20TokenholderActionCreatorRole,
STRATEGY,
address(mockProtocol),
0,
Expand All @@ -299,17 +266,17 @@ contract CreateActionBySig is ERC20TokenholderActionCreatorTest {

function test_CheckNonceIncrements() public {
(uint8 v, bytes32 r, bytes32 s) = createOffchainSignature(tokenHolder1PrivateKey);
assertEq(erc20TokenholderActionCreator.nonces(tokenHolder1, ILlamaCore.createActionBySig.selector), 0);
assertEq(erc20TokenholderActionCreator.nonces(tokenHolder1, TokenholderActionCreator.createActionBySig.selector), 0);
createActionBySig(v, r, s);
assertEq(erc20TokenholderActionCreator.nonces(tokenHolder1, ILlamaCore.createActionBySig.selector), 1);
assertEq(erc20TokenholderActionCreator.nonces(tokenHolder1, TokenholderActionCreator.createActionBySig.selector), 1);
}

function test_OperationCannotBeReplayed() public {
(uint8 v, bytes32 r, bytes32 s) = createOffchainSignature(tokenHolder1PrivateKey);
createActionBySig(v, r, s);
// Invalid Signature error since the recovered signer address during the second call is not the same as
// policyholder since nonce has increased.
vm.expectRevert(ILlamaCore.InvalidSignature.selector);
vm.expectRevert(TokenholderActionCreator.InvalidSignature.selector);
createActionBySig(v, r, s);
}

Expand All @@ -318,27 +285,27 @@ contract CreateActionBySig is ERC20TokenholderActionCreatorTest {
(uint8 v, bytes32 r, bytes32 s) = createOffchainSignature(randomSignerPrivateKey);
// Invalid Signature error since the recovered signer address is not the same as the policyholder passed in as
// parameter.
vm.expectRevert(ILlamaCore.InvalidSignature.selector);
vm.expectRevert(TokenholderActionCreator.InvalidSignature.selector);
createActionBySig(v, r, s);
}

function test_RevertIf_SignerIsZeroAddress() public {
(uint8 v, bytes32 r, bytes32 s) = createOffchainSignature(tokenHolder1PrivateKey);
// Invalid Signature error since the recovered signer address is zero address due to invalid signature values
// (v,r,s).
vm.expectRevert(ILlamaCore.InvalidSignature.selector);
vm.expectRevert(TokenholderActionCreator.InvalidSignature.selector);
createActionBySig((v + 1), r, s);
}

function test_RevertIf_PolicyholderIncrementsNonce() public {
(uint8 v, bytes32 r, bytes32 s) = createOffchainSignature(tokenHolder1PrivateKey);

vm.prank(tokenHolder1);
erc20TokenholderActionCreator.incrementNonce(ILlamaCore.createActionBySig.selector);
erc20TokenholderActionCreator.incrementNonce(TokenholderActionCreator.createActionBySig.selector);

// Invalid Signature error since the recovered signer address during the call is not the same as policyholder
// since nonce has increased.
vm.expectRevert(ILlamaCore.InvalidSignature.selector);
vm.expectRevert(TokenholderActionCreator.InvalidSignature.selector);
createActionBySig(v, r, s);
}
}
Expand All @@ -350,8 +317,8 @@ contract CancelAction is ERC20TokenholderActionCreatorTest {
function setUp() public virtual override {
ERC20TokenholderActionCreatorTest.setUp();

// Assigns Policy to TokenholderActionCreator.
_initRoleSetRoleHolderSetRolePermissionToTokenholderActionCreator();
// Assigns Permission to TokenholderActionCreator.
_setRolePermissionToTokenholderActionCreator();

// Mint tokens to tokenholder so that they can create action.
erc20VotesToken.mint(tokenHolder1, ERC20_CREATION_THRESHOLD);
Expand All @@ -364,14 +331,12 @@ contract CancelAction is ERC20TokenholderActionCreatorTest {
bytes memory data = abi.encodeCall(mockProtocol.pause, (true));

vm.prank(tokenHolder1);
actionId = erc20TokenholderActionCreator.createAction(
erc20TokenholderActionCreatorRole, STRATEGY, address(mockProtocol), 0, data, ""
);
actionId = erc20TokenholderActionCreator.createAction(STRATEGY, address(mockProtocol), 0, data, "");

actionInfo = ActionInfo(
actionId,
address(erc20TokenholderActionCreator),
erc20TokenholderActionCreatorRole,
tokenVotingActionCreatorRole,
STRATEGY,
address(mockProtocol),
0,
Expand Down Expand Up @@ -401,8 +366,8 @@ contract CancelActionBySig is ERC20TokenholderActionCreatorTest {
function setUp() public virtual override {
ERC20TokenholderActionCreatorTest.setUp();

// Assigns Policy to TokenholderActionCreator.
_initRoleSetRoleHolderSetRolePermissionToTokenholderActionCreator();
// Assigns Permission to TokenholderActionCreator.
_setRolePermissionToTokenholderActionCreator();

// Mint tokens to tokenholder so that they can create action.
erc20VotesToken.mint(tokenHolder1, ERC20_CREATION_THRESHOLD);
Expand All @@ -415,14 +380,12 @@ contract CancelActionBySig is ERC20TokenholderActionCreatorTest {
bytes memory data = abi.encodeCall(mockProtocol.pause, (true));

vm.prank(tokenHolder1);
actionId = erc20TokenholderActionCreator.createAction(
erc20TokenholderActionCreatorRole, STRATEGY, address(mockProtocol), 0, data, ""
);
actionId = erc20TokenholderActionCreator.createAction(STRATEGY, address(mockProtocol), 0, data, "");

actionInfo = ActionInfo(
actionId,
address(erc20TokenholderActionCreator),
erc20TokenholderActionCreatorRole,
tokenVotingActionCreatorRole,
STRATEGY,
address(mockProtocol),
0,
Expand All @@ -435,12 +398,12 @@ contract CancelActionBySig is ERC20TokenholderActionCreatorTest {
view
returns (uint8 v, bytes32 r, bytes32 s)
{
LlamaCoreSigUtils.CancelActionBySig memory cancelAction = LlamaCoreSigUtils.CancelActionBySig({
LlamaCoreSigUtils.CancelAction memory cancelAction = LlamaCoreSigUtils.CancelAction({
tokenHolder: tokenHolder1,
actionInfo: _actionInfo,
nonce: erc20TokenholderActionCreator.nonces(tokenHolder1, ILlamaCore.cancelActionBySig.selector)
nonce: erc20TokenholderActionCreator.nonces(tokenHolder1, TokenholderActionCreator.cancelActionBySig.selector)
});
bytes32 digest = getCancelActionBySigTypedDataHash(cancelAction);
bytes32 digest = getCancelActionTypedDataHash(cancelAction);
(v, r, s) = vm.sign(privateKey, digest);
}

Expand All @@ -463,17 +426,17 @@ contract CancelActionBySig is ERC20TokenholderActionCreatorTest {
function test_CheckNonceIncrements() public {
(uint8 v, bytes32 r, bytes32 s) = createOffchainSignature(actionInfo, tokenHolder1PrivateKey);

assertEq(erc20TokenholderActionCreator.nonces(tokenHolder1, ILlamaCore.cancelActionBySig.selector), 0);
assertEq(erc20TokenholderActionCreator.nonces(tokenHolder1, TokenholderActionCreator.cancelActionBySig.selector), 0);
cancelActionBySig(actionInfo, v, r, s);
assertEq(erc20TokenholderActionCreator.nonces(tokenHolder1, ILlamaCore.cancelActionBySig.selector), 1);
assertEq(erc20TokenholderActionCreator.nonces(tokenHolder1, TokenholderActionCreator.cancelActionBySig.selector), 1);
}

function test_OperationCannotBeReplayed() public {
(uint8 v, bytes32 r, bytes32 s) = createOffchainSignature(actionInfo, tokenHolder1PrivateKey);
cancelActionBySig(actionInfo, v, r, s);
// Invalid Signature error since the recovered signer address during the second call is not the same as policyholder
// since nonce has increased.
vm.expectRevert(ILlamaCore.InvalidSignature.selector);
vm.expectRevert(TokenholderActionCreator.InvalidSignature.selector);
cancelActionBySig(actionInfo, v, r, s);
}

Expand All @@ -482,27 +445,27 @@ contract CancelActionBySig is ERC20TokenholderActionCreatorTest {
(uint8 v, bytes32 r, bytes32 s) = createOffchainSignature(actionInfo, randomSignerPrivateKey);
// Invalid Signature error since the recovered signer address is not the same as the policyholder passed in as
// parameter.
vm.expectRevert(ILlamaCore.InvalidSignature.selector);
vm.expectRevert(TokenholderActionCreator.InvalidSignature.selector);
cancelActionBySig(actionInfo, v, r, s);
}

function test_RevertIf_SignerIsZeroAddress() public {
(uint8 v, bytes32 r, bytes32 s) = createOffchainSignature(actionInfo, tokenHolder1PrivateKey);
// Invalid Signature error since the recovered signer address is zero address due to invalid signature values
// (v,r,s).
vm.expectRevert(ILlamaCore.InvalidSignature.selector);
vm.expectRevert(TokenholderActionCreator.InvalidSignature.selector);
cancelActionBySig(actionInfo, (v + 1), r, s);
}

function test_RevertIf_PolicyholderIncrementsNonce() public {
(uint8 v, bytes32 r, bytes32 s) = createOffchainSignature(actionInfo, tokenHolder1PrivateKey);

vm.prank(tokenHolder1);
erc20TokenholderActionCreator.incrementNonce(ILlamaCore.cancelActionBySig.selector);
erc20TokenholderActionCreator.incrementNonce(TokenholderActionCreator.cancelActionBySig.selector);

// Invalid Signature error since the recovered signer address during the call is not the same as policyholder
// since nonce has increased.
vm.expectRevert(ILlamaCore.InvalidSignature.selector);
vm.expectRevert(TokenholderActionCreator.InvalidSignature.selector);
cancelActionBySig(actionInfo, v, r, s);
}
}
Expand Down
Loading

0 comments on commit 797689c

Please sign in to comment.