Skip to content

Commit c5551cc

Browse files
alexb5dhalexb5dh
authored
XDC: Fix duplicate signature quorum bypass (#11379)
* `QuorumCertificateManager` deduplication fix * Parallize `CountValidSignatures` and make it static * Introduce `CertificateManagerBase`, move `CountValidSignatures` there and reuse it in `TimeoutCertificateManager` * Code cleanup * Remove `CertificateManagerBase`, move `CountValidSignatures` to `VotesManager` and fix `GetValidSignatures` there * PR feedback & code cleanup * Build fix * PR feedback * spelling * Code cleanup * Revert `VotesManager.GetValidSignatures` changes
1 parent bb16311 commit c5551cc

9 files changed

Lines changed: 153 additions & 72 deletions

‎src/Nethermind/Nethermind.Xdc.Test/Helpers/XdcTestHelper.cs‎

Lines changed: 23 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -1,9 +1,10 @@
1-
// SPDX-FileCopyrightText: 2025 Demerzel Solutions Limited
1+
// SPDX-FileCopyrightText: 2026 Demerzel Solutions Limited
22
// SPDX-License-Identifier: LGPL-3.0-only
33

44
using Nethermind.Core;
55
using Nethermind.Core.Crypto;
66
using Nethermind.Crypto;
7+
using Nethermind.Int256;
78
using Nethermind.Serialization.Rlp;
89
using Nethermind.Xdc.RLP;
910
using Nethermind.Xdc.Types;
@@ -72,6 +73,27 @@ public static Vote BuildSignedVote(BlockRoundInfo info, ulong gap, PrivateKey ke
7273
return vote;
7374
}
7475

76+
/// <summary>
77+
/// Produces a byte-distinct but cryptographically valid alternative signature for the same
78+
/// message and private key by exploiting secp256k1 malleability: (r, s) → (r, N−s, flipped v).
79+
/// Both signatures recover to the same signer address, so they represent a single vote from
80+
/// one validator regardless of how many byte-distinct copies exist.
81+
/// </summary>
82+
public static Signature CreateMalleableSignature(Signature original)
83+
{
84+
ReadOnlySpan<byte> bytes = original.Bytes; // 64 bytes: r (0..31), s (32..63)
85+
86+
UInt256 s = new(bytes[32..], true);
87+
UInt256 sNew = SecP256k1Curve.N - s;
88+
89+
byte[] result = new byte[65];
90+
bytes[..32].CopyTo(result); // r unchanged
91+
sNew.ToBigEndian(result.AsSpan(32, 32));
92+
result[64] = original.V == 27 ? (byte)28 : (byte)27; // flip recovery id
93+
94+
return new Signature(result);
95+
}
96+
7597
public static byte[] BuildV1ExtraData(Address[] addresses)
7698
{
7799
byte[] extraData = new byte[XdcConstants.ExtraVanity + addresses.Length * Address.Size + XdcConstants.ExtraSeal];

‎src/Nethermind/Nethermind.Xdc.Test/ModuleTests/VotesManagerTests.cs‎

Lines changed: 9 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -1,6 +1,7 @@
1-
// SPDX-FileCopyrightText: 2025 Demerzel Solutions Limited
1+
// SPDX-FileCopyrightText: 2026 Demerzel Solutions Limited
22
// SPDX-License-Identifier: LGPL-3.0-only
33

4+
using System;
45
using Nethermind.Blockchain;
56
using Nethermind.Consensus;
67
using Nethermind.Core;
@@ -28,6 +29,7 @@ public static IEnumerable<TestCaseData> HandleVoteCases()
2829
PrivateKey[] keysForMasternodes = keys.Take(20).ToArray();
2930
PrivateKey[] extraKeys = keys.Skip(20).ToArray();
3031
Address[] masternodes = keysForMasternodes.Select(k => k.Address).ToArray();
32+
int quorumCount = (int)Math.Ceiling(keysForMasternodes.Length * 0.667);
3133

3234
ulong currentRound = 1;
3335
XdcBlockHeader header = Build.A.XdcBlockHeader()
@@ -49,6 +51,12 @@ public static IEnumerable<TestCaseData> HandleVoteCases()
4951
for (int i = 0; i < keysForVotes.Length - 3; i++) votesWithDiffGap.Add(XdcTestHelper.BuildSignedVote(info, 450, keysForVotes[i]));
5052
for (int i = keysForVotes.Length - 3; i < keysForVotes.Length; i++) votesWithDiffGap.Add(XdcTestHelper.BuildSignedVote(info, 451, keysForVotes[i]));
5153
yield return new TestCaseData(masternodes, header, currentRound, votesWithDiffGap.ToArray(), info, 0);
54+
55+
//N byte-distinct votes but only N-1 unique addresses (keys[0] signs twice via ECDSA malleability)
56+
Vote[] legitimateVotes = [.. keysForMasternodes.Take(quorumCount - 1).Select(k => XdcTestHelper.BuildSignedVote(info, 450, k))];
57+
Signature malleableSig = XdcTestHelper.CreateMalleableSignature(legitimateVotes[0].Signature!);
58+
Vote malleableVote = new(info, 450) { Signature = malleableSig, Signer = legitimateVotes[0].Signer };
59+
yield return new TestCaseData(masternodes, header, currentRound, (Vote[])[.. legitimateVotes, malleableVote], info, 0);
5260
}
5361

5462
[TestCaseSource(nameof(HandleVoteCases))]

‎src/Nethermind/Nethermind.Xdc.Test/QuorumCertificateManagerTest.cs‎

Lines changed: 12 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -1,6 +1,7 @@
1-
// SPDX-FileCopyrightText: 2025 Demerzel Solutions Limited
1+
// SPDX-FileCopyrightText: 2026 Demerzel Solutions Limited
22
// SPDX-License-Identifier: LGPL-3.0-only
33

4+
using System;
45
using Nethermind.Blockchain;
56
using Nethermind.Core;
67
using Nethermind.Core.Crypto;
@@ -51,13 +52,15 @@ public static IEnumerable<TestCaseData> QcCases()
5152
{
5253
XdcBlockHeaderBuilder headerBuilder = Build.A.XdcBlockHeader().WithGeneratedExtraConsensusData();
5354
PrivateKeyGenerator keyBuilder = new();
54-
//Base valid control case
5555
PrivateKey[] keys = keyBuilder.Generate(20).ToArray();
5656
IEnumerable<Address> masterNodes = keys.Select(k => k.Address);
57+
int quorumCount = (int)Math.Ceiling(keys.Length * 0.667);
58+
59+
//Base valid control case
5760
yield return new TestCaseData(XdcTestHelper.CreateQc(new BlockRoundInfo(headerBuilder.TestObject.Hash!, 1, 1), 0, keys), headerBuilder, keys.Select(k => k.Address), true);
5861

5962
//Not enough signatures
60-
yield return new TestCaseData(XdcTestHelper.CreateQc(new BlockRoundInfo(headerBuilder.TestObject.Hash!, 1, 1), 0, keys.Take(13).ToArray()), headerBuilder, keys.Select(k => k.Address), false);
63+
yield return new TestCaseData(XdcTestHelper.CreateQc(new BlockRoundInfo(headerBuilder.TestObject.Hash!, 1, 1), 0, [.. keys.Take(quorumCount - 1)]), headerBuilder, keys.Select(k => k.Address), false);
6164

6265
//1 Vote is not master node
6366
yield return new TestCaseData(XdcTestHelper.CreateQc(new BlockRoundInfo(headerBuilder.TestObject.Hash!, 1, 1), 0, keys), headerBuilder, keys.Skip(1).Select(k => k.Address), false);
@@ -73,6 +76,12 @@ public static IEnumerable<TestCaseData> QcCases()
7376

7477
//Wrong round number in QC
7578
yield return new TestCaseData(XdcTestHelper.CreateQc(new BlockRoundInfo(headerBuilder.TestObject.Hash!, 0, 1), 0, keys), headerBuilder, masterNodes, false);
79+
80+
//N byte-distinct signatures but only N-1 unique signer addresses (keys[0] signs twice via ECDSA malleability)
81+
BlockRoundInfo roundInfo = new(headerBuilder.TestObject.Hash!, 1, 1);
82+
Signature[] sigs = XdcTestHelper.CreateVoteSignatures(roundInfo, 0, [.. keys.Take(quorumCount - 1)]);
83+
Signature malleable = XdcTestHelper.CreateMalleableSignature(sigs[0]);
84+
yield return new TestCaseData(new QuorumCertificate(roundInfo, [.. sigs, malleable], 0), headerBuilder, masterNodes, false);
7685
}
7786

7887
[TestCaseSource(nameof(QcCases))]

‎src/Nethermind/Nethermind.Xdc.Test/SyncInfoManagerTests.cs‎

Lines changed: 4 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -163,7 +163,7 @@ public void VerifySyncInfo_WhenAllVerificationsPass_ReturnsTrue()
163163
ILogManager logManager = Substitute.For<ILogManager>();
164164

165165
qcManager.VerifyCertificate(qc, out Arg.Any<string>()).Returns(true);
166-
timeoutManager.VerifyTimeoutCertificate(tc, out Arg.Any<string>()).Returns(true);
166+
timeoutManager.VerifyTimeoutCertificate(tc, out Arg.Any<string?>()).Returns(true);
167167

168168
SyncInfoManager manager = new(xdcContext, qcManager, timeoutManager, logManager);
169169

@@ -194,7 +194,7 @@ public void VerifySyncInfo_WhenBothCertificatesAreHigher_ReturnsTrue()
194194
ILogManager logManager = Substitute.For<ILogManager>();
195195

196196
qcManager.VerifyCertificate(qc, out Arg.Any<string>()).Returns(true);
197-
timeoutManager.VerifyTimeoutCertificate(tc, out Arg.Any<string>()).Returns(true);
197+
timeoutManager.VerifyTimeoutCertificate(tc, out Arg.Any<string?>()).Returns(true);
198198

199199
SyncInfoManager manager = new(xdcContext, qcManager, timeoutManager, logManager);
200200

@@ -225,7 +225,7 @@ public void VerifySyncInfo_WhenOnlyQuorumCertificateIsHigher_ReturnsTrue()
225225
ILogManager logManager = Substitute.For<ILogManager>();
226226

227227
qcManager.VerifyCertificate(qc, out Arg.Any<string>()).Returns(true);
228-
timeoutManager.VerifyTimeoutCertificate(tc, out Arg.Any<string>()).Returns(true);
228+
timeoutManager.VerifyTimeoutCertificate(tc, out Arg.Any<string?>()).Returns(true);
229229

230230
SyncInfoManager manager = new(xdcContext, qcManager, timeoutManager, logManager);
231231

@@ -256,7 +256,7 @@ public void VerifySyncInfo_WhenOnlyTimeoutCertificateIsHigher_ReturnsTrue()
256256
ILogManager logManager = Substitute.For<ILogManager>();
257257

258258
qcManager.VerifyCertificate(qc, out Arg.Any<string>()).Returns(true);
259-
timeoutManager.VerifyTimeoutCertificate(tc, out Arg.Any<string>()).Returns(true);
259+
timeoutManager.VerifyTimeoutCertificate(tc, out Arg.Any<string?>()).Returns(true);
260260

261261
SyncInfoManager manager = new(xdcContext, qcManager, timeoutManager, logManager);
262262

‎src/Nethermind/Nethermind.Xdc.Test/TimeoutCertificateManagerTests.cs‎

Lines changed: 12 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -1,4 +1,4 @@
1-
// SPDX-FileCopyrightText: 2025 Demerzel Solutions Limited
1+
// SPDX-FileCopyrightText: 2026 Demerzel Solutions Limited
22
// SPDX-License-Identifier: LGPL-3.0-only
33

44
using Nethermind.Blockchain;
@@ -23,7 +23,6 @@ namespace Nethermind.Xdc.Test;
2323
[Parallelizable(ParallelScope.All)]
2424
public class TimeoutCertificateManagerTests
2525
{
26-
2726
[Test]
2827
public void VerifyTC_NullCert_Throws()
2928
{
@@ -102,22 +101,30 @@ public static IEnumerable<TestCaseData> TcCases()
102101
PrivateKeyGenerator keyBuilder = new();
103102
PrivateKey[] keys = keyBuilder.Generate(20).ToArray();
104103
IEnumerable<Address> masterNodes = keys.Select(k => k.Address);
104+
int quorumCount = (int)Math.Ceiling(keys.Length * 0.667);
105105

106106
// Base case
107107
yield return new TestCaseData(BuildTimeoutCertificate(keys), masterNodes, true);
108108

109109
// Insufficient signature count
110-
PrivateKey[] notEnoughKeys = keys.Take(13).ToArray();
110+
PrivateKey[] notEnoughKeys = [.. keys.Take(quorumCount - 1)];
111111
yield return new TestCaseData(BuildTimeoutCertificate(notEnoughKeys), masterNodes, false);
112112

113113
// Duplicated signatures still should fail if not enough
114-
yield return new TestCaseData(BuildTimeoutCertificate(notEnoughKeys.Concat(notEnoughKeys).ToArray()), masterNodes, false);
114+
yield return new TestCaseData(BuildTimeoutCertificate([.. notEnoughKeys, .. notEnoughKeys]), masterNodes, false);
115115

116116
// Sufficient signature count
117-
yield return new TestCaseData(BuildTimeoutCertificate(keys.Take(14).ToArray()), masterNodes, true);
117+
yield return new TestCaseData(BuildTimeoutCertificate([.. keys.Take(quorumCount)]), masterNodes, true);
118118

119119
// Signer not in master nodes
120120
yield return new TestCaseData(BuildTimeoutCertificate(keys), keys.Skip(1).Select(k => k.Address), false);
121+
122+
//N byte-distinct signatures but only N-1 unique signer addresses (keys[0] signs twice via ECDSA malleability)
123+
EthereumEcdsa ecdsa = new(0);
124+
ValueHash256 msgHash = TimeoutCertificateManager.ComputeTimeoutMsgHash(1, 0);
125+
Signature[] sigs = [.. keys.Take(quorumCount - 1).Select(k => ecdsa.Sign(k, msgHash))];
126+
Signature malleable = XdcTestHelper.CreateMalleableSignature(sigs[0]);
127+
yield return new TestCaseData(new TimeoutCertificate(1, [.. sigs, malleable], 0), masterNodes, false);
121128
}
122129

123130
[TestCaseSource(nameof(TcCases))]

‎src/Nethermind/Nethermind.Xdc/ITimeoutCertificateManager.cs‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -13,6 +13,6 @@ public interface ITimeoutCertificateManager
1313
Task HandleTimeoutVote(Timeout timeout);
1414
void OnCountdownTimer();
1515
void ProcessTimeoutCertificate(TimeoutCertificate timeoutCertificate);
16-
bool VerifyTimeoutCertificate(TimeoutCertificate timeoutCertificate, out string errorMessage);
16+
bool VerifyTimeoutCertificate(TimeoutCertificate timeoutCertificate, out string? errorMessage);
1717
long GetTimeoutsCount(Timeout timeout);
1818
}

‎src/Nethermind/Nethermind.Xdc/QuorumCertificateManager.cs‎

Lines changed: 25 additions & 34 deletions
Original file line numberDiff line numberDiff line change
@@ -1,4 +1,4 @@
1-
// SPDX-FileCopyrightText: 2025 Demerzel Solutions Limited
1+
// SPDX-FileCopyrightText: 2026 Demerzel Solutions Limited
22
// SPDX-License-Identifier: LGPL-3.0-only
33

44
using Nethermind.Blockchain;
@@ -13,8 +13,6 @@
1313
using Nethermind.Xdc.Types;
1414
using System;
1515
using System.Diagnostics.CodeAnalysis;
16-
using System.Linq;
17-
using System.Threading.Tasks;
1816

1917
namespace Nethermind.Xdc;
2018

@@ -32,8 +30,7 @@ internal class QuorumCertificateManager(
3230
private ILogger _logger = logManager.GetClassLogger<QuorumCertificateManager>();
3331

3432
private ISpecProvider _specProvider { get; } = xdcConfig;
35-
private readonly EthereumEcdsa _ethereumEcdsa = new(0);
36-
private readonly static VoteDecoder _voteDecoder = new();
33+
private static readonly VoteDecoder _voteDecoder = new();
3734

3835
public QuorumCertificate HighestKnownCertificate => _context.HighestQC;
3936
public QuorumCertificate LockCertificate => _context.LockQC;
@@ -45,18 +42,16 @@ public void CommitCertificate(QuorumCertificate qc)
4542
_context.HighestQC = qc;
4643
}
4744

48-
XdcBlockHeader proposedBlockHeader = (XdcBlockHeader)_blockTree.FindHeader(qc.ProposedBlockInfo.Hash);
49-
if (proposedBlockHeader is null)
50-
throw new IncomingMessageBlockNotFoundException(qc.ProposedBlockInfo.Hash, qc.ProposedBlockInfo.BlockNumber);
45+
XdcBlockHeader proposedBlockHeader = (XdcBlockHeader)_blockTree.FindHeader(qc.ProposedBlockInfo.Hash)
46+
?? throw new IncomingMessageBlockNotFoundException(qc.ProposedBlockInfo.Hash, qc.ProposedBlockInfo.BlockNumber);
5147

5248
IXdcReleaseSpec spec = _specProvider.GetXdcSpec(proposedBlockHeader, _context.CurrentRound);
5349

5450
//Can only look for a QC in proposed block after the switch block
5551
if (proposedBlockHeader.Number > spec.SwitchBlock)
5652
{
57-
QuorumCertificate? parentQc = proposedBlockHeader.ExtraConsensusData?.QuorumCert;
58-
if (parentQc is null)
59-
throw new BlockchainException("QC is targeting a block without required consensus data.");
53+
QuorumCertificate? parentQc = proposedBlockHeader.ExtraConsensusData?.QuorumCert
54+
?? throw new BlockchainException("QC is targeting a block without required consensus data.");
6055

6156
if (_context.LockQC is null || parentQc.ProposedBlockInfo.Round > _context.LockQC.ProposedBlockInfo.Round)
6257
{
@@ -68,7 +63,6 @@ public void CommitCertificate(QuorumCertificate qc)
6863
{
6964
if (_logger.IsWarn) _logger.Warn($"Could not commit block ({proposedBlockHeader.Hash}). {error}");
7065
}
71-
7266
}
7367

7468
if (qc.ProposedBlockInfo.Round >= _context.CurrentRound)
@@ -124,7 +118,7 @@ private bool CommitBlock(XdcBlockHeader proposedBlockHeader, ulong proposedRound
124118
return false;
125119
}
126120

127-
//We will normally commit twice - once when QC vote finished and once when we receive new block containing the same QC most likely
121+
//We will normally commit twice - once when QC vote finished and once when we receive new block containing the same QC most likely
128122
if (_context.HighestCommitBlock is not null && grandParentHeader.Hash == _context.HighestCommitBlock.Hash)
129123
{
130124
error = null;
@@ -171,35 +165,31 @@ public bool VerifyCertificate(QuorumCertificate qc, XdcBlockHeader certificateTa
171165
return false;
172166
}
173167

174-
//Possible optimize here
175-
Signature[] uniqueSignatures = qc.Signatures.Distinct().ToArray();
176-
177168
ulong qcRound = qc.ProposedBlockInfo.Round;
178169
IXdcReleaseSpec spec = _specProvider.GetXdcSpec(certificateTarget, qcRound);
179170
double certificateThreshold = spec.CertificateThreshold;
180171
double required = Math.Ceiling(epochSwitchInfo.Masternodes.Length * certificateThreshold);
181-
if ((qcRound > 0) && (uniqueSignatures.Length < required))
182-
{
183-
error = $"Number of votes ({uniqueSignatures.Length}/{epochSwitchInfo.Masternodes.Length}) does not meet threshold of {certificateThreshold}";
184-
return false;
185-
}
186172

187-
ValueHash256 voteHash = VoteHash(qc.ProposedBlockInfo, qc.GapNumber);
188-
bool allValid = true;
189-
Parallel.ForEach(uniqueSignatures, (s, state) =>
173+
if (qcRound > 0)
190174
{
191-
Address signer = _ethereumEcdsa.RecoverAddress(s, voteHash);
192-
if (!epochSwitchInfo.Masternodes.Contains(signer))
175+
(Address[] masternodes, Signature[] signatures) = (epochSwitchInfo.Masternodes, qc.Signatures);
176+
if (signatures.Length < required)
193177
{
194-
allValid = false;
195-
state.Stop();
178+
error = $"Number of signatures ({signatures.Length}) does not meet threshold of {required}";
179+
return false;
196180
}
197-
});
198181

199-
if (!allValid)
200-
{
201-
error = $"Quorum certificate contains one or more invalid vote signatures";
202-
return false;
182+
ValueHash256 voteHash = VoteHash(qc.ProposedBlockInfo, qc.GapNumber);
183+
if (VotesManager.CountValidSignatures(masternodes, signatures, voteHash, out error) is not { } signCount)
184+
{
185+
return false;
186+
}
187+
188+
if (signCount < required)
189+
{
190+
error = $"Number of votes ({signCount}/{masternodes.Length}) does not meet threshold of {certificateThreshold}";
191+
return false;
192+
}
203193
}
204194

205195
long epochSwitchNumber = epochSwitchInfo.EpochSwitchBlockInfo.BlockNumber;
@@ -232,7 +222,8 @@ public bool VerifyCertificate(QuorumCertificate qc, XdcBlockHeader certificateTa
232222
error = null;
233223
return true;
234224
}
235-
private ValueHash256 VoteHash(BlockRoundInfo proposedBlockInfo, ulong gapNumber)
225+
226+
private static ValueHash256 VoteHash(BlockRoundInfo proposedBlockInfo, ulong gapNumber)
236227
{
237228
KeccakRlpStream stream = new();
238229
_voteDecoder.Encode(stream, new Vote(proposedBlockInfo, gapNumber), RlpBehaviors.ForSealing);

0 commit comments

Comments
 (0)