Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Check vsize credentials delta in ReissuanceAsync #7822

Closed

Conversation

onvej-sl
Copy link
Contributor

Fixes #7821.

@lontivero
Copy link
Collaborator

What a bug!

I have no permission to contribute to this branch. Could you please add this UT in WalletWasabi.Tests/UnitTests/WabiSabi/Backend/PostRequests/CredentialReissuanceTests.cs please?

using NBitcoin;
using System.Linq;
using System.Threading;
using System.Threading.Tasks;
using WalletWasabi.Tests.Helpers;
using WalletWasabi.WabiSabi.Backend;
using WalletWasabi.WabiSabi.Backend.Models;
using WalletWasabi.WabiSabi.Backend.Rounds;
using Xunit;

namespace WalletWasabi.Tests.UnitTests.WabiSabi.Backend.PostRequests;

public class CredentialReissuanceTest
{
	[Fact]
	public async Task ReissueExactDeltaAmountAsync()
	{
		WabiSabiConfig cfg = new();
		var round = WabiSabiFactory.CreateRound(cfg);
		round.SetPhase(Phase.OutputRegistration);
		var alice = WabiSabiFactory.CreateAlice(round);
		round.Alices.Add(alice);
		using Arena arena = await ArenaBuilder.From(cfg).CreateAndStartAsync(round);

		// Step 1. Create credentials
		var (amClient, vsClient, amIssuer, vsIssuer, amZeroCredentials, vsZeroCredentials) = WabiSabiFactory.CreateWabiSabiClientsAndIssuers(round);

		var amountsToRequest = new[]
			{alice.CalculateRemainingAmountCredentials(round.FeeRate, round.CoordinationFeeRate).Satoshi};
		var (amCredentialRequest, amValid) = amClient.CreateRequest(
			amountsToRequest,
			amZeroCredentials, // FIXME doesn't make much sense
			CancellationToken.None);
		var startingVsizeCredentialAmount = 100L; // any number is okay here for this test
		var (vsCredentialRequest, weValid) = vsClient.CreateRequest(
			new[] { startingVsizeCredentialAmount },
			vsZeroCredentials, // FIXME doesn't make much sense
			CancellationToken.None);

		var amResp = amIssuer.HandleRequest(amCredentialRequest);
		var weResp = vsIssuer.HandleRequest(vsCredentialRequest);
		var amountCredentials = amClient.HandleResponse(amResp, amValid).ToArray();
		var vsizeCredentials = vsClient.HandleResponse(weResp, weValid).ToArray();

		// Step 2. Try to request an invalid reissuance
		var arenaClient = WabiSabiFactory.CreateArenaClient(arena);
		var ex = await Assert.ThrowsAsync<WabiSabiProtocolException>(async () =>
			await arenaClient.ReissueCredentialAsync(
				round.Id,
				amountsToRequest,
				vsizeCredentials.Select(x => 2 * x.Value), // we request the double than what we can
				amountCredentials,
				vsizeCredentials,
				CancellationToken.None
			));
		Assert.Equal(WabiSabiProtocolErrorCode.DeltaNotZero, ex.ErrorCode);
		await arena.StopAsync(CancellationToken.None);
	}
}

Btw, feel free to modify it if you want.

Copy link
Collaborator

@molnard molnard left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Look a reasonable change to me. @lontivero you have the final call on this.

@lontivero
Copy link
Collaborator

I don't think we can merge this too soon. The first that is missing for me here is to understand where we are dropping the excess of vsize credentials. This is something we should remember but honestly I don't.

The rest are just minor things: a few UTs (at least two) and implement the validation on the client side too as is the case of other validations. Finally testing.

@lontivero lontivero mentioned this pull request Apr 22, 2022
@lontivero lontivero closed this Apr 22, 2022
@onvej-sl onvej-sl deleted the onvej-sl/vsize-credentials-fix branch May 2, 2022 13:05
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Vulnerability in credential reissuance
3 participants