diff --git a/contracts/DLCManager.sol b/contracts/DLCManager.sol index 6104235..1ca0f2f 100644 --- a/contracts/DLCManager.sol +++ b/contracts/DLCManager.sol @@ -16,6 +16,7 @@ import "./DLCLinkLibrary.sol"; import "./IBTC.sol"; import "@chainlink/contracts/src/v0.8/shared/interfaces/AggregatorV3Interface.sol"; +import "@openzeppelin/contracts/utils/Strings.sol"; /** * @author DLC.Link 2024 @@ -34,6 +35,7 @@ contract DLCManager is using DLCLink for DLCLink.DLC; using DLCLink for DLCLink.DLCStatus; using SafeERC20 for IBTC; + using Strings for string; //////////////////////////////////////////////////////////////// // STATE VARIABLES // @@ -70,11 +72,13 @@ contract DLCManager is mapping(address => bool) private _whitelistedAddresses; bool public porEnabled; AggregatorV3Interface public dlcBTCPoRFeed; - mapping(address => mapping(bytes32 => bool)) private _seenSigners; + mapping(address => mapping(bytes32 => bool)) private _seenSigners; // deprecated uint256 public totalValueMinted; mapping(address => uint256) private _btcMintFeeRates; mapping(address => uint256) private _btcRedeemFeeRates; - uint256[36] __gap; + mapping(string => bool) private _processedPendingTransactions; + mapping(string => bool) private _processedFundedTransactions; + uint256[34] __gap; //////////////////////////////////////////////////////////////// // ERRORS // @@ -99,6 +103,7 @@ contract DLCManager is error DuplicateSigner(address signer); error SignerNotApproved(address signer); error ClosingFundedVault(); + error TransactionAlreadyProcessed(string txId, string functionString); error InvalidRange(); error NotOwner(); @@ -110,6 +115,7 @@ contract DLCManager is error FeeRateOutOfBounds(uint256 feeRate); error UnderCollateralized(uint256 newValueLocked, uint256 valueMinted); error NotEnoughReserves(uint256 reserves, uint256 amount); + error InvalidFunctionString(string functionString); //////////////////////////////////////////////////////////////// // MODIFIERS // @@ -249,23 +255,40 @@ contract DLCManager is } /** - * @notice Checks the 'signatures' of Attestors for a given 'message'. + * @notice Checks the signatures of Attestors for a given transaction. * @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, from unique signers. - * @param message Original message that was signed. - * @param signatures Byte array of at least 'threshold' number of signatures. + * @dev Checks if the btc transaction has already been processed for this function signature. + * @param uuid Unique identifier for the transaction + * @param btcTxId Bitcoin transaction ID + * @param newValueLocked New value locked in the transaction + * @param signatures Byte array of at least 'threshold' number of signatures + * @param functionString String identifying the function being called */ function _attestorMultisigIsValid( - bytes memory message, - bytes[] memory signatures - ) internal { + bytes32 uuid, + string memory btcTxId, + uint256 newValueLocked, + bytes[] memory signatures, + string memory functionString + ) internal view { if (signatures.length < _threshold) revert NotEnoughSignatures(); bytes32 prefixedMessageHash = ECDSAUpgradeable.toEthSignedMessageHash( - keccak256(message) + keccak256(abi.encode(uuid, btcTxId, functionString, newValueLocked)) ); + if ( + (Strings.equal(functionString, "set-status-pending") && + _processedPendingTransactions[btcTxId]) || + (Strings.equal(functionString, "set-status-funded") && + _processedFundedTransactions[btcTxId]) + ) { + revert TransactionAlreadyProcessed(btcTxId, functionString); + } + + address[] memory seenSigners = new address[](signatures.length); for (uint256 i = 0; i < signatures.length; i++) { address attestorPubKey = ECDSAUpgradeable.recover( prefixedMessageHash, @@ -274,18 +297,15 @@ contract DLCManager is if (!hasRole(APPROVED_SIGNER, attestorPubKey)) { revert InvalidSigner(); } - _checkSignerUnique(attestorPubKey, prefixedMessageHash); - } - } - function _checkSignerUnique( - address attestorPubKey, - bytes32 messageHash - ) internal { - if (_seenSigners[attestorPubKey][messageHash]) { - revert DuplicateSigner(attestorPubKey); + // Check for duplicates in already seen signers + for (uint256 j = 0; j < i; j++) { + if (seenSigners[j] == attestorPubKey) { + revert DuplicateSigner(attestorPubKey); + } + } + seenSigners[i] = attestorPubKey; } - _seenSigners[attestorPubKey][messageHash] = true; } /** @@ -401,8 +421,11 @@ contract DLCManager is uint256 newValueLocked ) external whenNotPaused onlyApprovedSigners { _attestorMultisigIsValid( - abi.encode(uuid, btcTxId, "set-status-funded", newValueLocked), - signatures + uuid, + btcTxId, + newValueLocked, + signatures, + "set-status-funded" ); DLCLink.DLC storage dlc = dlcs[dlcIDsByUUID[uuid]]; @@ -440,6 +463,8 @@ contract DLCManager is _mintTokens(dlc.creator, amountToMint); } + _processedFundedTransactions[btcTxId] = true; + emit SetStatusFunded( uuid, btcTxId, @@ -466,8 +491,11 @@ contract DLCManager is uint256 newValueLocked ) external whenNotPaused onlyApprovedSigners { _attestorMultisigIsValid( - abi.encode(uuid, wdTxId, "set-status-pending", newValueLocked), - signatures + uuid, + wdTxId, + newValueLocked, + signatures, + "set-status-pending" ); DLCLink.DLC storage dlc = dlcs[dlcIDsByUUID[uuid]]; @@ -481,6 +509,8 @@ contract DLCManager is dlc.wdTxId = wdTxId; dlc.taprootPubKey = taprootPubKey; + _processedPendingTransactions[wdTxId] = true; + emit SetStatusPending( uuid, wdTxId, diff --git a/test/DLCManager.test.js b/test/DLCManager.test.js index c4efe46..b649200 100644 --- a/test/DLCManager.test.js +++ b/test/DLCManager.test.js @@ -446,6 +446,93 @@ describe('DLCManager', () => { }); }); + describe('signature checks on SSP and SSF', () => { + let uuid; + beforeEach(async () => { + await whitelistAddress(dlcManager, user); + + const tx = await dlcManager.connect(user).setupVault(); + const receipt = await tx.wait(); + const event = receipt.events[0]; + const decodedEvent = dlcManager.interface.parseLog(event); + uuid = decodedEvent.args.uuid; + + await setSigners(dlcManager, attestors); + }); + + it('should revert on nonce-manipulated signatures from the same signer', async () => { + 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); + + // Change threshold and add the new signer + await dlcManager.connect(deployer).setThreshold(4); + await setSigners(dlcManager, [maliciousAttestor]); + + // Sign pending status + const signatureBytesForPending = + await getMultipleSignaturesForSameAttestorAndMessage( + { + uuid: _uuid, + btcTxId, + functionString: 'set-status-pending', + newLockedAmount: 0, + }, + maliciousSigner, + 4 + ); + + // Fund with just one signature + 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'); + }); + }); + describe('setStatusFunded', async () => { let uuid; beforeEach(async () => { @@ -522,7 +609,7 @@ describe('DLCManager', () => { { uuid, btcTxId, - functionString: 'post-close-dlc', + functionString: 'set-status-pending', newLockedAmount: valueLocked, }, attestors, @@ -536,79 +623,6 @@ describe('DLCManager', () => { ).to.be.revertedWithCustomError(dlcManager, 'InvalidSigner'); }); - it('should revert on nonce-manipulated signatures from the same signer', async () => { - const existingBalance = await iBTC.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); - - // Change threshold and add the new signer - await dlcManager.connect(deployer).setThreshold(4); - await setSigners(dlcManager, [maliciousAttestor]); - - // Sign pending status - const signatureBytesForPending = - await getMultipleSignaturesForSameAttestorAndMessage( - { - uuid: _uuid, - btcTxId, - functionString: 'set-status-pending', - newLockedAmount: 0, - }, - maliciousSigner, - 4 - ); - - // Fund with just one signature - 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( { @@ -770,6 +784,90 @@ describe('DLCManager', () => { expect(event.args.uuid).to.equal(uuid); expect(event.args.btcTxId).to.equal(btcTxId); }); + + it('Revert on attempted replay attacks when attacker trying to reuse sigs on same function and same btcTxId', async () => { + // Setup: Add more attestors to have more than threshold + const attestor4 = accounts[9]; + const attestor5 = accounts[10]; + const attestor6 = accounts[11]; + + // Add the additional attestors + await setSigners(dlcManager, [attestor4, attestor5, attestor6]); + + // Complete the normal flow first + const signatureBytes = await getSignatures( + { + uuid, + btcTxId, // Original btcTxId from beforeEach + functionString: 'set-status-funded', + newLockedAmount: valueLocked, + }, + [attestor1, attestor2, attestor3], + 3 + ); + + await dlcManager + .connect(attestor1) + .setStatusFunded(uuid, btcTxId, signatureBytes, valueLocked); + + // Verify initial state + let dlc = await dlcManager.getDLC(uuid); + expect(dlc.status).to.equal(1); // DLCStatus.FUNDED + expect(await iBTC.balanceOf(user.address)).to.equal(valueLocked); + + const replayAttestors = [attestor4, attestor5, attestor6]; + + const pendingSignatures1 = await getSignatures( + { + uuid, + btcTxId, + functionString: 'set-status-pending', + newLockedAmount: 0, + }, + replayAttestors, + 3 + ); + + await expect( + dlcManager + .connect(attestor4) + .setStatusPending( + uuid, + btcTxId, + pendingSignatures1, + mockTaprootPubkey, + 0 + ) + ).to.be.revertedWithCustomError( + dlcManager, + 'TransactionAlreadyProcessed' + ); + + const fundedSignatures1 = await getSignatures( + { + uuid, + btcTxId, + functionString: 'set-status-funded', + newLockedAmount: valueLocked * 2, + }, + replayAttestors, + 3 + ); + + await expect( + dlcManager + .connect(attestor4) + .setStatusFunded( + uuid, + btcTxId, + fundedSignatures1, + valueLocked * 2 + ) + ).to.be.revertedWithCustomError( + dlcManager, + 'TransactionAlreadyProcessed' + ); + }); }); describe('totalValueMinted', async () => { @@ -842,7 +940,7 @@ describe('DLCManager', () => { const signatureBytesForPending = await getSignatures( { uuid: uuid2, - btcTxId, + btcTxId: btcTxId2, functionString: 'set-status-pending', newLockedAmount: 0, }, @@ -853,7 +951,7 @@ describe('DLCManager', () => { .connect(attestor1) .setStatusPending( uuid2, - btcTxId, + btcTxId2, signatureBytesForPending, mockTaprootPubkey, 0 @@ -863,7 +961,7 @@ describe('DLCManager', () => { const signatureBytesForFunding = await getSignatures( { uuid: uuid2, - btcTxId, + btcTxId: btcTxId2, functionString: 'set-status-funded', newLockedAmount: valueLocked, }, @@ -874,7 +972,7 @@ describe('DLCManager', () => { .connect(attestor1) .setStatusFunded( uuid2, - btcTxId, + btcTxId2, signatureBytesForFunding, valueLocked );