Skip to content

Commit

Permalink
fix: nonce manipulation (#116)
Browse files Browse the repository at this point in the history 
* fix: nonce manipulation
  • Loading branch information
@scolear
scolear authored Sep 26, 2024
1 parent 7b901b6 commit fe25947
Show file tree
Hide file tree
Showing 3 changed files with 177 additions and 6 deletions.
23 changes: 19 additions & 4 deletions contracts/DLCManager.sol
View file
Original file line number Diff line number Diff line change
Expand Up @@ -85,6 +85,7 @@ contract DLCManager is
error NotEnoughSignatures();
error InvalidSigner();
error DuplicateSignature();
error DuplicateSigner(address signer);
error SignerNotApproved(address signer);
error ClosingFundedVault();

Expand Down Expand Up @@ -213,7 +214,7 @@ contract DLCManager is
* @notice Checks the 'signatures' of Attestors for a given 'message'.
* @dev Recalculates the hash to make sure the signatures are for the same message.
* @dev Uses OpenZeppelin's ECDSA library to recover the public keys from the signatures.
* @dev Signatures must be unique.
* @dev Signatures must be unique, from unique signers.
* @param message Original message that was signed.
* @param signatures Byte array of at least 'threshold' number of signatures.
*/
Expand All @@ -222,20 +223,23 @@ contract DLCManager is
bytes[] memory signatures
) internal view {
if (signatures.length < _threshold) revert NotEnoughSignatures();
if (_hasDuplicates(signatures)) revert DuplicateSignature();

bytes32 prefixedMessageHash = ECDSAUpgradeable.toEthSignedMessageHash(
keccak256(message)
);

if (_hasDuplicates(signatures)) revert DuplicateSignature();
address[] memory seenSigners = new address[](signatures.length); // to store unique signers

for (uint256 i = 0; i < signatures.length; i++) {
address attestorPubKey = ECDSAUpgradeable.recover(
prefixedMessageHash,
signatures[i]
);
if (!hasRole(APPROVED_SIGNER, attestorPubKey))
if (!hasRole(APPROVED_SIGNER, attestorPubKey)) {
revert InvalidSigner();
}
_checkSignerUnique(seenSigners, attestorPubKey);
seenSigners[i] = attestorPubKey;
}
}

Expand All @@ -258,6 +262,17 @@ contract DLCManager is
return false;
}

function _checkSignerUnique(
address[] memory seenSigners,
address attestorPubKey
) internal pure {
for (uint256 j = 0; j < seenSigners.length; j++) {
if (seenSigners[j] == attestorPubKey) {
revert DuplicateSigner(attestorPubKey);
}
}
}

function _mintTokens(address to, uint256 amount) internal {
dlcBTC.mint(to, amount);
emit Mint(to, amount);
Expand Down
87 changes: 86 additions & 1 deletion test/DLCManager.test.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -3,7 +3,11 @@ const { ethers } = require('hardhat');
const hardhat = require('hardhat');
const crypto = require('crypto');

const { getSignatures, setSigners } = require('./utils');
const {
getSignatures,
setSigners,
getMultipleSignaturesForSameAttestorAndMessage,
} = require('./utils');

async function whitelistAddress(dlcManager, user) {
await dlcManager.whitelistAddress(user.address);
Expand Down Expand Up @@ -457,6 +461,87 @@ describe('DLCManager', () => {
).to.be.revertedWithCustomError(dlcManager, 'InvalidSigner');
});

it('should revert on nonce-manipulated signatures from the same signer', async () => {
const existingBalance = await dlcBtc.balanceOf(user.address);
const deposit = 100000000; // 1 BTC
const tx = await dlcManager.connect(user).setupVault();
const receipt = await tx.wait();
const _uuid = await receipt.events[0].args.uuid;

// Hardhat account #9
let maliciousAttestor = new ethers.Wallet(
ethers.utils.arrayify(
'0x2a871d0798f97d79848a013d4936a73bf4cc922c825d33c1cf7073dff6d409c6'
)
);

const maliciousSigner = new ethers.Wallet(
maliciousAttestor,
ethers.provider
);
attestors.push(maliciousSigner);

console.log(
`Malicious signer address: ${maliciousSigner.address}\n`
);
// Change threshold and add the new signer
await dlcManager.connect(deployer).setThreshold(4);
await setSigners(dlcManager, [maliciousAttestor]);

// Sign pending status
console.log(`Wallet threshold: ${await dlcManager.getThreshold()}`);
console.log('Signing pending status:');
const signatureBytesForPending =
await getMultipleSignaturesForSameAttestorAndMessage(
{
uuid: _uuid,
btcTxId,
functionString: 'set-status-pending',
newLockedAmount: 0,
},
maliciousSigner,
4
);

// Fund with just one signature
console.log('\n');
console.log('Signing funded status:');

const signatureBytesForFunding =
await getMultipleSignaturesForSameAttestorAndMessage(
{
uuid: _uuid,
btcTxId,
functionString: 'set-status-funded',
newLockedAmount: deposit,
},
maliciousSigner,
4
);
await expect(
dlcManager
.connect(maliciousSigner)
.setStatusPending(
_uuid,
btcTxId,
signatureBytesForPending,
mockTaprootPubkey,
0
)
).to.be.revertedWithCustomError(dlcManager, 'DuplicateSigner');

await expect(
dlcManager
.connect(maliciousSigner)
.setStatusFunded(
_uuid,
btcTxId,
signatureBytesForFunding,
deposit
)
).to.be.revertedWithCustomError(dlcManager, 'DuplicateSigner');
});

it('reverts if DLC is not in the right state', async () => {
const signatureBytes = await getSignatures(
{
Expand Down
73 changes: 72 additions & 1 deletion test/utils.js
View file
Original file line number Diff line number Diff line change
@@ -1,4 +1,6 @@
const { ethers } = require('hardhat');
const crypto = require('crypto');
const secp256k1 = require('secp256k1');

async function getSignatures(message, attestors, numberOfSignatures) {
const hashedOriginalMessage = ethers.utils.keccak256(
Expand Down Expand Up @@ -33,4 +35,73 @@ async function setSigners(dlcManager, attestors) {
}
}

module.exports = { getSignatures, setSigners };
function signMessageCustom(privateKey, messageHash, nonce) {
// Prepend the Ethereum signed message prefix
const prefixBytes = ethers.utils.toUtf8Bytes(
'\x19Ethereum Signed Message:\n32'
);
const prefixedMessageBytes = new Uint8Array(
prefixBytes.length + messageHash.length
);
prefixedMessageBytes.set(prefixBytes);
prefixedMessageBytes.set(messageHash, prefixBytes.length);

// Hash the prefixed message
const prefixedMessageHash = ethers.utils.keccak256(prefixedMessageBytes);
const prefixedMessageHashBytes = ethers.utils.arrayify(prefixedMessageHash);

const signature = secp256k1.ecdsaSign(
prefixedMessageHashBytes,
privateKey,
{ noncefn: () => nonce }
);
return ethers.utils.joinSignature({
r: '0x' + Buffer.from(signature.signature.slice(0, 32)).toString('hex'),
s:
'0x' +
Buffer.from(signature.signature.slice(32, 64)).toString('hex'),
v: signature.recid + 27,
});
}

async function getMultipleSignaturesForSameAttestorAndMessage(
message,
attestor,
numberOfSignatures
) {
const hashedOriginalMessage = ethers.utils.keccak256(
ethers.utils.defaultAbiCoder.encode(
['bytes32', 'string', 'string', 'uint256'],
[
message.uuid,
message.btcTxId,
message.functionString,
message.newLockedAmount,
]
)
);

let signatureBytes = [];
console.log(
`Signing ${numberOfSignatures} messages: ${hashedOriginalMessage} with private key: ${attestor.privateKey}`
);

for (let i = 0; i < numberOfSignatures; i++) {
const randomNonce = crypto.randomBytes(32);
const sig = await signMessageCustom(
ethers.utils.arrayify(attestor.privateKey),
ethers.utils.arrayify(hashedOriginalMessage),
randomNonce
);
console.log(`Signature ${i}: ${sig}`);
signatureBytes.push(ethers.utils.arrayify(sig));
}
// Convert signatures from strings to bytes
return signatureBytes;
}

module.exports = {
getSignatures,
setSigners,
getMultipleSignaturesForSameAttestorAndMessage,
};

0 comments on commit fe25947

Please sign in to comment.