diff --git a/src/Util/Signatures.sol b/src/Util/Signatures.sol index 2706d9eb..585109b7 100644 --- a/src/Util/Signatures.sol +++ b/src/Util/Signatures.sol @@ -1,9 +1,9 @@ // SPDX-License-Identifier: MIT pragma solidity ^0.8.19; -library Signatures { - error InvalidSignatureLength(); +import "@openzeppelin/contracts/utils/cryptography/ECDSA.sol"; +library Signatures { /** Hash of the message to sign */ function getPostageMessageHash( bytes32 _chunkAddr, @@ -25,7 +25,7 @@ library Signatures { bytes32 messageHash = getPostageMessageHash(_chunkAddr, _postageId, _index, _timeStamp); bytes32 ethMessageHash = getEthSignedMessageHash(messageHash); - return recoverSigner(ethMessageHash, _signature) == _signer; + return verifySignature(ethMessageHash, _signature, _signer); } function getEthSignedMessageHash(bytes32 _messageHash) internal pure returns (bytes32) { @@ -36,38 +36,13 @@ library Signatures { return keccak256(abi.encodePacked("\x19Ethereum Signed Message:\n32", _messageHash)); } - function recoverSigner( + function verifySignature( bytes32 _ethSignedMessageHash, // it has to be prefixed message: https://ethereum.stackexchange.com/questions/19582/does-ecrecover-in-solidity-expects-the-x19ethereum-signed-message-n-prefix/21037 - bytes memory _signature - ) internal pure returns (address) { - (bytes32 r, bytes32 s, uint8 v) = splitSignature(_signature); - - return ecrecover(_ethSignedMessageHash, v, r, s); - } - - function splitSignature(bytes memory sig) internal pure returns (bytes32 r_, bytes32 s_, uint8 v_) { - if (sig.length != 65) { - revert InvalidSignatureLength(); - } - - assembly { - /* - verbose explanation: https://ethereum.stackexchange.com/questions/135591/split-signature-function-in-solidity-by-example-docs - First 32 bytes stores the length of the signature - add(sig, 32) = pointer of sig + 32 - effectively, skips first 32 bytes of signature - mload(p) loads next 32 bytes starting at the memory address p into memory - */ - - // first 32 bytes, after the length prefix - r_ := mload(add(sig, 32)) - // second 32 bytes - s_ := mload(add(sig, 64)) - // final byte (first byte of the next 32 bytes) - v_ := byte(0, mload(add(sig, 96))) - } - - // implicitly return (r, s, v) + bytes memory _signature, + address _signer + ) internal pure returns (bool) { + (address recovered, ECDSA.RecoverError error) = ECDSA.tryRecover(_ethSignedMessageHash, _signature); + return error == ECDSA.RecoverError.NoError && recovered == _signer; } function getSocMessageHash(bytes32 _identifier, bytes32 _chunkAddr) internal pure returns (bytes32) { @@ -83,6 +58,6 @@ library Signatures { bytes32 messageHash = getSocMessageHash(_identifier, _chunkAddr); bytes32 ethMessageHash = getEthSignedMessageHash(messageHash); - return recoverSigner(ethMessageHash, _signature) == _signer; + return verifySignature(ethMessageHash, _signature, _signer); } } diff --git a/src/test/SignaturesHarness.sol b/src/test/SignaturesHarness.sol new file mode 100644 index 00000000..a388c9e1 --- /dev/null +++ b/src/test/SignaturesHarness.sol @@ -0,0 +1,27 @@ +// SPDX-License-Identifier: MIT +pragma solidity ^0.8.19; + +import "../Util/Signatures.sol"; + +/// @dev Thin wrapper for unit-testing the Signatures library. +contract SignaturesHarness { + function socVerify( + address signer, + bytes memory signature, + bytes32 identifier, + bytes32 chunkAddr + ) external pure returns (bool) { + return Signatures.socVerify(signer, signature, identifier, chunkAddr); + } + + function postageVerify( + address signer, + bytes memory signature, + bytes32 chunkAddr, + bytes32 postageId, + uint64 index, + uint64 timeStamp + ) external pure returns (bool) { + return Signatures.postageVerify(signer, signature, chunkAddr, postageId, index, timeStamp); + } +} diff --git a/test/Redistribution.test.ts b/test/Redistribution.test.ts index 443e461c..9c3860f5 100644 --- a/test/Redistribution.test.ts +++ b/test/Redistribution.test.ts @@ -1173,6 +1173,18 @@ describe('Redistribution', function () { ).to.be.revertedWith(errors.claim.socVerificationFailed); }); + it('rejects SOC address(0) signer bypass at claim', async function () { + const { proofParams } = await generatedSampling(true); + const socProof = proofParams.proof1.socProof![0]; + + socProof.signer = ethers.constants.AddressZero; + socProof.signature = '0x' + '00'.repeat(65); + + await expect( + r_node_5.claim(proofParams.proof1, proofParams.proof2, proofParams.proofLast) + ).to.be.revertedWith(errors.claim.socVerificationFailed); + }); + it('SOC attachment does not match with witness', async function () { const { proofParams } = await generatedSampling(true); diff --git a/test/Signatures.test.ts b/test/Signatures.test.ts new file mode 100644 index 00000000..25d4d001 --- /dev/null +++ b/test/Signatures.test.ts @@ -0,0 +1,86 @@ +import { expect } from './util/chai'; +import { ethers } from 'hardhat'; +import { hexlify } from 'ethers/lib/utils'; +import { randomBytes } from 'crypto'; + +describe('Signatures', () => { + let harness: Awaited>; + + async function deployHarness() { + const factory = await ethers.getContractFactory('SignaturesHarness'); + return factory.deploy(); + } + + before(async () => { + harness = await deployHarness(); + }); + + it('rejects SOC verification bypass via address(0) and malformed signature', async () => { + const identifier = randomBytes(32); + const chunkAddr = randomBytes(32); + const malformedSignature = '0x' + '00'.repeat(65); + + const verified = await harness.socVerify( + ethers.constants.AddressZero, + malformedSignature, + hexlify(identifier), + hexlify(chunkAddr) + ); + + expect(verified).to.be.false; + }); + + it('rejects SOC verification with invalid signature length', async () => { + const identifier = randomBytes(32); + const chunkAddr = randomBytes(32); + const shortSignature = '0x' + '00'.repeat(64); + + const verified = await harness.socVerify( + ethers.Wallet.createRandom().address, + shortSignature, + hexlify(identifier), + hexlify(chunkAddr) + ); + + expect(verified).to.be.false; + }); + + it('rejects SOC verification with high-s malleable signature', async () => { + const wallet = ethers.Wallet.createRandom(); + const identifier = randomBytes(32); + const chunkAddr = randomBytes(32); + const messageHash = ethers.utils.solidityKeccak256(['bytes32', 'bytes32'], [identifier, chunkAddr]); + const signature = await wallet.signMessage(ethers.utils.arrayify(messageHash)); + + const { r, s, v } = ethers.utils.splitSignature(signature); + const malleableS = ethers.BigNumber.from('0xFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFEBAAEDCE6AF48A03BBFD25E8CD0364141').sub( + ethers.BigNumber.from(s) + ); + const malleableSignature = ethers.utils.hexConcat([ + r, + ethers.utils.hexZeroPad(malleableS.toHexString(), 32), + ethers.utils.hexlify(v === 27 ? 28 : 27), + ]); + + const verified = await harness.socVerify( + wallet.address, + malleableSignature, + hexlify(identifier), + hexlify(chunkAddr) + ); + + expect(verified).to.be.false; + }); + + it('accepts a valid SOC signature', async () => { + const wallet = ethers.Wallet.createRandom(); + const identifier = randomBytes(32); + const chunkAddr = randomBytes(32); + const messageHash = ethers.utils.solidityKeccak256(['bytes32', 'bytes32'], [identifier, chunkAddr]); + const signature = await wallet.signMessage(ethers.utils.arrayify(messageHash)); + + const verified = await harness.socVerify(wallet.address, signature, hexlify(identifier), hexlify(chunkAddr)); + + expect(verified).to.be.true; + }); +});