Skip to content

Commit

Permalink
fix: ledger-signed message signature adjustment
Browse files Browse the repository at this point in the history
  • Loading branch information
@kupermind
kupermind committed May 4, 2023
1 parent f14491a commit 784d312
Show file tree
Hide file tree
Showing 3 changed files with 45 additions and 9 deletions.
11 changes: 6 additions & 5 deletions contracts/utils/OperatorSignedHashes.sol
View file
Original file line number Diff line number Diff line change
Expand Up @@ -105,10 +105,11 @@ contract OperatorSignedHashes {

// Decode the signature
uint8 v = uint8(signature[64]);
// Check if v is zero because it was signed by the ledger
if (v == 0 && operator.code.length == 0) {
// For the correct ecrecover() function execution, the v value must be set to {0,1} + 27
// If v is set to just 0 or 1 when signing by the EOA, it is most likely signed by the ledger and must be adjusted
if (v < 2 && operator.code.length == 0) {
// In case of a non-contract, adjust v to follow the standard ecrecover case
v = 27;
v += 27;
}
bytes32 r;
bytes32 s;
Expand All @@ -120,15 +121,15 @@ contract OperatorSignedHashes {

address recOperator;
// Go through signature cases based on the value of v
if (v == 0) {
if (v == 2) {
// Contract signature case, where the address of the contract is encoded into r
recOperator = address(uint160(uint256(r)));

// Check for the signature validity in the contract
if (ISignatureValidator(recOperator).isValidSignature(msgHash, signature) != MAGIC_VALUE) {
revert HashNotValidated(recOperator, msgHash, signature);
}
} else if (v == 1) {
} else if (v == 3) {
// Case of an approved hash, where the address of the operator is encoded into r
recOperator = address(uint160(uint256(r)));

Expand Down
7 changes: 7 additions & 0 deletions test/Deployment.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -2,10 +2,14 @@

const { expect } = require("chai");
const { ethers } = require("hardhat");
const helpers = require("@nomicfoundation/hardhat-network-helpers");

describe("Deployment", function () {
context("Deployment", async function () {
it("Deployment steps", async function () {
// Take a snapshot of the current state of the blockchain
const snapshot = await helpers.takeSnapshot();

const signers = await ethers.getSigners();
// EOA
const EOA = signers[0];
Expand Down Expand Up @@ -80,6 +84,9 @@ describe("Deployment", function () {
expect(await serviceRegistry.owner()).to.equal(timelock.address);
expect(await registriesManager.owner()).to.equal(timelock.address);
expect(await serviceManager.owner()).to.equal(timelock.address);

// Restore a previous state of blockchain
snapshot.restore();
});
});
});
Expand Down
36 changes: 32 additions & 4 deletions test/ServiceManagementWithOperatorSignatures.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,7 @@

const { expect } = require("chai");
const { ethers } = require("hardhat");
const helpers = require("@nomicfoundation/hardhat-network-helpers");

describe("ServiceManagementWithOperatorSignatures", function () {
let componentRegistry;
Expand Down Expand Up @@ -106,6 +107,9 @@ describe("ServiceManagementWithOperatorSignatures", function () {

context("Redeployment of services", async function () {
it("Changing the service owner and redeploying with the new multisig owner", async function () {
// Take a snapshot of the current state of the blockchain
const snapshot = await helpers.takeSnapshot();

const agentInstances = [signers[2], signers[3], signers[4], signers[5]];
const agentInstancesAddresses = [signers[2].address, signers[3].address, signers[4].address, signers[5].address];
const maxThreshold = 1;
Expand Down Expand Up @@ -309,9 +313,15 @@ describe("ServiceManagementWithOperatorSignatures", function () {
// Check that the service is deployed
const service = await serviceRegistry.getService(serviceId);
expect(service.state).to.equal(4);

// Restore a previous state of blockchain
snapshot.restore();
});

it("Several scenarios with errors when unbonding using operator signatures with the token-secured service", async function () {
// Take a snapshot of the current state of the blockchain
const snapshot = await helpers.takeSnapshot();

const agentInstances = [signers[2], signers[3], signers[4], signers[5]];
const agentInstancesAddresses = [signers[2].address, signers[3].address, signers[4].address, signers[5].address];
const maxThreshold = 1;
Expand Down Expand Up @@ -395,14 +405,23 @@ describe("ServiceManagementWithOperatorSignatures", function () {

// Unbond the agent instance in order to update the service using a pre-signed operator message
// Case 1. Simulate the unbond when the message was signed by the ledger (change v value to zero)
const ledgerSignatureBytes = signatureBytes.substring(0, signatureBytes.length - 2) + "00";
// Take last byte of the v value and subtract 27
const lastByte = signatureBytes.slice(-2);
const lastByteValue = parseInt(lastByte, 16) - 27;
let ledgerSignatureBytes;
if (lastByteValue == 0) {
ledgerSignatureBytes = signatureBytes.substring(0, signatureBytes.length - 2) + "00";
} else {
ledgerSignatureBytes = signatureBytes.substring(0, signatureBytes.length - 2) + "01";
}

result = await serviceManager.connect(serviceOwner).callStatic.unbondWithSignature(operator.address, serviceId, ledgerSignatureBytes);
expect(result.success).to.be.true;

// Case 2. Approve the hash and provide the signature to follow the approved hash path
// Build the signature to follow the approved hash path
const approveSignatureBytes = "0x000000000000000000000000" + operator.address.slice(2) +
"0000000000000000000000000000000000000000000000000000000000000000" + "01";
"0000000000000000000000000000000000000000000000000000000000000000" + "03";

// Try to unbond agent instances on behalf of the operator without the hash being pre-approved
await expect(
Expand All @@ -422,7 +441,7 @@ describe("ServiceManagementWithOperatorSignatures", function () {
// Case 3. Provide the signature to follow the isValidSignature() path when the operator is the contract
// Build the signature to follow the contract hash verification path
const verifySignatureBytes = "0x000000000000000000000000" + operatorContract.address.slice(2) +
"0000000000000000000000000000000000000000000000000000000000000000" + "00";
"0000000000000000000000000000000000000000000000000000000000000000" + "02";

// Try to unbond agent instances on behalf of the operator without the hash being validated
await expect(
Expand All @@ -441,7 +460,7 @@ describe("ServiceManagementWithOperatorSignatures", function () {

// Try to use a different operator address
await expect(
serviceManager.connect(serviceOwner).callStatic.unbondWithSignature(operator.address, serviceId, verifySignatureBytes)
serviceManager.connect(serviceOwner).callStatic.unbondWithSignature(serviceOwnerAddress, serviceId, signatureBytes)
).to.be.revertedWithCustomError(serviceManager, "WrongOperatorAddress");

// Perform the actual unbond
Expand Down Expand Up @@ -547,9 +566,15 @@ describe("ServiceManagementWithOperatorSignatures", function () {
// Check that the service is deployed
const service = await serviceRegistry.getService(serviceId);
expect(service.state).to.equal(4);

// Restore a previous state of blockchain
snapshot.restore();
});

it("Redeploy service in one shot via the timelock contract", async function () {
// Take a snapshot of the current state of the blockchain
const snapshot = await helpers.takeSnapshot();

const agentInstances = [signers[2], signers[3], signers[4], signers[5]];
const agentInstancesAddresses = [signers[2].address, signers[3].address, signers[4].address, signers[5].address];
const maxThreshold = 1;
Expand Down Expand Up @@ -757,6 +782,9 @@ describe("ServiceManagementWithOperatorSignatures", function () {
// Check that the service is deployed
const service = await serviceRegistry.getService(serviceId);
expect(service.state).to.equal(4);

// Restore a previous state of blockchain
snapshot.restore();
});
});
});

0 comments on commit 784d312

Please sign in to comment.