Skip to content

Commit

Permalink
fix: signer uniqueness checking (#122)
Browse files Browse the repository at this point in the history
  • Loading branch information
@scolear
scolear authored Oct 16, 2024
1 parent 83af649 commit f64c9a4
Show file tree
Hide file tree
Showing 2 changed files with 49 additions and 83 deletions.
42 changes: 10 additions & 32 deletions contracts/DLCManager.sol
View file
Original file line number Diff line number Diff line change
Expand Up @@ -68,7 +68,8 @@ contract DLCManager is
mapping(address => bool) private _whitelistedAddresses;
bool public porEnabled;
AggregatorV3Interface public dlcBTCPoRFeed;
uint256[40] __gap;
mapping(address => mapping(bytes32 => bool)) private _seenSigners;
uint256[39] __gap;

////////////////////////////////////////////////////////////////
// ERRORS //
Expand Down Expand Up @@ -230,14 +231,12 @@ contract DLCManager is
function _attestorMultisigIsValid(
bytes memory message,
bytes[] memory signatures
) internal view {
) internal {
if (signatures.length < _threshold) revert NotEnoughSignatures();
if (_hasDuplicates(signatures)) revert DuplicateSignature();

bytes32 prefixedMessageHash = ECDSAUpgradeable.toEthSignedMessageHash(
keccak256(message)
);
address[] memory seenSigners = new address[](signatures.length); // to store unique signers

for (uint256 i = 0; i < signatures.length; i++) {
address attestorPubKey = ECDSAUpgradeable.recover(
Expand All @@ -247,39 +246,18 @@ contract DLCManager is
if (!hasRole(APPROVED_SIGNER, attestorPubKey)) {
revert InvalidSigner();
}
_checkSignerUnique(seenSigners, attestorPubKey);
seenSigners[i] = attestorPubKey;
_checkSignerUnique(attestorPubKey, prefixedMessageHash);
}
}

/**
* @notice Checks for duplicate values in an array.
* @dev Used to check for duplicate signatures.
* @param signatures Array of signatures.
* @return bool True if there are duplicates, false otherwise.
*/
function _hasDuplicates(
bytes[] memory signatures
) internal pure returns (bool) {
for (uint i = 0; i < signatures.length - 1; i++) {
for (uint j = i + 1; j < signatures.length; j++) {
if (keccak256(signatures[i]) == keccak256(signatures[j])) {
return true;
}
}
}
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);
}
address attestorPubKey,
bytes32 messageHash
) internal {
if (_seenSigners[attestorPubKey][messageHash]) {
revert DuplicateSigner(attestorPubKey);
}
_seenSigners[attestorPubKey][messageHash] = true;
}

/**
Expand Down
90 changes: 39 additions & 51 deletions test/DLCManager.test.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -14,7 +14,7 @@ async function whitelistAddress(dlcManager, user) {
}

describe('DLCManager', () => {
let dlcManager, dlcBtc;
let dlcManager, dlcBtc, uuid;
let accounts, deployer, user, randomAccount, anotherAccount, protocol;
let attestor1, attestor2, attestor3;
let attestors;
Expand All @@ -23,6 +23,12 @@ describe('DLCManager', () => {
const btcTxId = '0x1234567890';
const btcTxId2 = '0x1234567891';
const someAddress = '0x1234567890123456789012345678901234567890';
const btcTxId3 = '0x1234567892';
const btcTxId4 = '0x1234567893';
const btcTxId5 = '0x1234567894';
const btcTxId6 = '0x1234567895';
const btcTxId7 = '0x1234567896';
const btcTxId8 = '0x1234567897';
const mockTaprootPubkey =
'0x1234567890123456789012345678901234567890123456789012345678901234';
let btcFeeRecipient = '0x000001';
Expand Down Expand Up @@ -655,48 +661,30 @@ describe('DLCManager', () => {
dlcManager
.connect(attestor1)
.setStatusFunded(uuid, btcTxId, signatureBytes, valueLocked)
).to.be.revertedWithCustomError(dlcManager, 'DuplicateSignature');
).to.be.revertedWithCustomError(dlcManager, 'DuplicateSigner');
});

it('reverts if deposit is too large', async () => {
it('reverts if attestors sign a different lockedAmount', async () => {
const signatureBytes = await getSignatures(
{
uuid,
btcTxId,
btcTxId: btcTxId8,
functionString: 'set-status-funded',
newLockedAmount: valueLocked * 100,
newLockedAmount: valueLocked + 100,
},
[attestor1, attestor1, attestor1],
[attestor1, attestor2, attestor3],
3
);
await expect(
dlcManager
.connect(attestor1)
.setStatusFunded(
uuid,
btcTxId,
btcTxId8,
signatureBytes,
valueLocked * 100
valueLocked
)
).to.be.revertedWithCustomError(dlcManager, 'DuplicateSignature');
});

it('reverts if deposit is too small', async () => {
const signatureBytes = await getSignatures(
{
uuid,
btcTxId,
functionString: 'set-status-funded',
newLockedAmount: 100,
},
[attestor1, attestor1, attestor1],
3
);
await expect(
dlcManager
.connect(attestor1)
.setStatusFunded(uuid, btcTxId, signatureBytes, 100)
).to.be.revertedWithCustomError(dlcManager, 'DuplicateSignature');
).to.be.revertedWithCustomError(dlcManager, 'InvalidSigner');
});

it('mints dlcBTC tokens to the user', async () => {
Expand Down Expand Up @@ -1063,7 +1051,7 @@ describe('DLCManager', () => {
const signatureBytesForPending = await getSignatures(
{
uuid,
btcTxId,
btcTxId: btcTxId2,
functionString: 'set-status-pending',
newLockedAmount: 0,
},
Expand All @@ -1074,7 +1062,7 @@ describe('DLCManager', () => {
.connect(attestor1)
.setStatusPending(
uuid,
btcTxId,
btcTxId2,
signatureBytesForPending,
mockTaprootPubkey,
0
Expand All @@ -1084,7 +1072,7 @@ describe('DLCManager', () => {
const signatureBytesForFunding = await getSignatures(
{
uuid,
btcTxId,
btcTxId: btcTxId2,
functionString: 'set-status-funded',
newLockedAmount: valueLocked / 2,
},
Expand All @@ -1095,7 +1083,7 @@ describe('DLCManager', () => {
.connect(attestor1)
.setStatusFunded(
uuid,
btcTxId,
btcTxId2,
signatureBytesForFunding,
valueLocked / 2
);
Expand All @@ -1118,7 +1106,7 @@ describe('DLCManager', () => {
const signatureBytesForPending = await getSignatures(
{
uuid,
btcTxId,
btcTxId: btcTxId3,
functionString: 'set-status-pending',
newLockedAmount: 0,
},
Expand All @@ -1129,7 +1117,7 @@ describe('DLCManager', () => {
.connect(attestor1)
.setStatusPending(
uuid,
btcTxId,
btcTxId3,
signatureBytesForPending,
mockTaprootPubkey,
0
Expand All @@ -1139,7 +1127,7 @@ describe('DLCManager', () => {
const signatureBytesForFunding = await getSignatures(
{
uuid,
btcTxId,
btcTxId: btcTxId3,
functionString: 'set-status-funded',
newLockedAmount: valueLocked / 2 - 1,
},
Expand All @@ -1151,7 +1139,7 @@ describe('DLCManager', () => {
.connect(attestor1)
.setStatusFunded(
uuid,
btcTxId,
btcTxId3,
signatureBytesForFunding,
valueLocked / 2 - 1
)
Expand All @@ -1166,7 +1154,7 @@ describe('DLCManager', () => {
const signatureBytesForPending = await getSignatures(
{
uuid,
btcTxId,
btcTxId: btcTxId4,
functionString: 'set-status-pending',
newLockedAmount: 0,
},
Expand All @@ -1177,7 +1165,7 @@ describe('DLCManager', () => {
.connect(attestor1)
.setStatusPending(
uuid,
btcTxId,
btcTxId4,
signatureBytesForPending,
mockTaprootPubkey,
0
Expand All @@ -1187,7 +1175,7 @@ describe('DLCManager', () => {
const signatureBytesForFunding = await getSignatures(
{
uuid,
btcTxId,
btcTxId: btcTxId4,
functionString: 'set-status-funded',
newLockedAmount: valueLocked - 1,
},
Expand All @@ -1199,7 +1187,7 @@ describe('DLCManager', () => {
.connect(attestor1)
.setStatusFunded(
uuid,
btcTxId,
btcTxId4,
signatureBytesForFunding,
valueLocked - 1
)
Expand All @@ -1222,7 +1210,7 @@ describe('DLCManager', () => {
const signatureBytesForPending = await getSignatures(
{
uuid,
btcTxId,
btcTxId: btcTxId5,
functionString: 'set-status-pending',
newLockedAmount: 0,
},
Expand All @@ -1233,7 +1221,7 @@ describe('DLCManager', () => {
.connect(attestor1)
.setStatusPending(
uuid,
btcTxId,
btcTxId5,
signatureBytesForPending,
mockTaprootPubkey,
0
Expand All @@ -1243,7 +1231,7 @@ describe('DLCManager', () => {
const signatureBytesForFunding = await getSignatures(
{
uuid,
btcTxId,
btcTxId: btcTxId5,
functionString: 'set-status-funded',
newLockedAmount: valueLocked,
},
Expand All @@ -1254,7 +1242,7 @@ describe('DLCManager', () => {
.connect(attestor1)
.setStatusFunded(
uuid,
btcTxId,
btcTxId5,
signatureBytesForFunding,
valueLocked
);
Expand All @@ -1266,7 +1254,7 @@ describe('DLCManager', () => {
const signatureBytesForPending = await getSignatures(
{
uuid,
btcTxId,
btcTxId: btcTxId6,
functionString: 'set-status-pending',
newLockedAmount: 0,
},
Expand All @@ -1277,7 +1265,7 @@ describe('DLCManager', () => {
.connect(attestor1)
.setStatusPending(
uuid,
btcTxId,
btcTxId6,
signatureBytesForPending,
mockTaprootPubkey,
0
Expand All @@ -1287,7 +1275,7 @@ describe('DLCManager', () => {
const signatureBytesForFunding = await getSignatures(
{
uuid,
btcTxId,
btcTxId: btcTxId6,
functionString: 'set-status-funded',
newLockedAmount: lockedAmountAfterDeposit,
},
Expand All @@ -1298,7 +1286,7 @@ describe('DLCManager', () => {
.connect(attestor1)
.setStatusFunded(
uuid,
btcTxId,
btcTxId6,
signatureBytesForFunding,
lockedAmountAfterDeposit
);
Expand All @@ -1318,7 +1306,7 @@ describe('DLCManager', () => {
const signatureBytesForPending = await getSignatures(
{
uuid,
btcTxId,
btcTxId: btcTxId7,
functionString: 'set-status-pending',
newLockedAmount: 0,
},
Expand All @@ -1329,7 +1317,7 @@ describe('DLCManager', () => {
.connect(attestor1)
.setStatusPending(
uuid,
btcTxId,
btcTxId7,
signatureBytesForPending,
mockTaprootPubkey,
0
Expand All @@ -1339,7 +1327,7 @@ describe('DLCManager', () => {
const signatureBytesForFunding = await getSignatures(
{
uuid,
btcTxId,
btcTxId: btcTxId7,
functionString: 'set-status-funded',
newLockedAmount: lockedAmountAfterDeposit,
},
Expand All @@ -1351,7 +1339,7 @@ describe('DLCManager', () => {
.connect(attestor1)
.setStatusFunded(
uuid,
btcTxId,
btcTxId7,
signatureBytesForFunding,
lockedAmountAfterDeposit
)
Expand Down

0 comments on commit f64c9a4

Please sign in to comment.