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.

Sign up for GitHub

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

Jump to bottom

wallet: check auction TXs for dust and null address before sending #479

Merged
merged 1 commit into from Aug 7, 2020
+42 −1
Merged

wallet: check auction TXs for dust and null address before sending #479

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
13 changes: 13 additions & 0 deletions lib/wallet/wallet.js
View file
Open in desktop
Expand Up @@ -3400,6 +3400,19 @@ class Wallet extends EventEmitter {
if (ancestors.size + 1 > this.maxAncestors)
throw new Error('TX exceeds maximum unconfirmed ancestors.');

for (const output of tx.outputs) {
if (output.isDust())
throw new Error('Output is dust.');

if (output.value > 0) {
if (!output.address)
throw new Error('Cannot send to unknown address.');

if (output.address.isNull())
Copy link
Contributor

Choose a reason for hiding this comment

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

What if somebody wants to provably burn some HNS? This would prevent somebody from using the API to do so. I don't see a consensus or policy rule that prevents such an output from existing.

Copy link
Member Author

Choose a reason for hiding this comment

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

I think they can use a nulldata address then. This check I think is to catch user error when an address isn't specified in the output and the null value remains:

hsd/lib/primitives/address.js

Lines 99 to 112 in 940f463

isNull() {
if (this.hash.length === 20)
return this.hash.equals(ZERO_HASH160);
if (this.hash.length === 32)
return this.hash.equals(consensus.ZERO_HASH);
for (let i = 0; i < this.hash.length; i++) {
if (this.hash[i] !== 0)
return false;
}
return true;
}

Copy link
Contributor

Choose a reason for hiding this comment

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

Ok, looking at the implementation of isNull, it doesn't check the address.version so this would still let somebody provably burn since its not checking Address.isNulldata

Copy link
Contributor

Choose a reason for hiding this comment

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

The same block of code is here:

hsd/lib/wallet/wallet.js

Line 3288 in 89d3ab4

if (output.isDust())

Copy link
Member Author

Choose a reason for hiding this comment

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

Right - if they used nulldata (which is just address version 31) they just couldn't ALSO use a ZERO_HASH as the data part.

throw new Error('Cannot send to null address.');
}
}

await this.wdb.addTX(tx);

this.logger.debug('Sending wallet tx (%s): %x', this.id, tx.hash());
Expand Down
12 changes: 12 additions & 0 deletions test/wallet-auction-test.js
View file
Open in desktop
Expand Up @@ -12,6 +12,7 @@ const Miner = require('../lib/mining/miner');
const WalletDB = require('../lib/wallet/walletdb');
const Network = require('../lib/protocol/network');
const rules = require('../lib/covenants/rules');
const Address = require('../lib/primitives/address');

const network = Network.get('regtest');
const NAME1 = rules.grindName(5, 2, network);
Expand Down Expand Up @@ -123,6 +124,17 @@ describe('Wallet Auction', function() {
}
});

it('should fail to send bid to null address', async () => {
const mtx = await winner.makeBid(NAME1, 1000, 2000, 0);
mtx.outputs[0].address = new Address();
await winner.fill(mtx);
await winner.finalize(mtx);

const fn = async () => await winner.sendMTX(mtx);

await assert.rejects(fn, {message: 'Cannot send to null address.'});
});

it('should fail to re-open auction during BIDDING phase', async () => {
let err;
try {
Expand Down
18 changes: 17 additions & 1 deletion test/wallet-http-test.js
View file
Open in desktop
Expand Up @@ -508,18 +508,34 @@ describe('Wallet HTTP', function() {
await assert.rejects(fn, {message: 'Lockup is required.'});
});

it('should send bid with 0 value and 0 lockup', async () => {
it('should send bid with 0 value and non-dust lockup', async () => {
await wallet.client.post(`/wallet/${wallet.id}/open`, {
name: name
});

await mineBlocks(treeInterval + 1, cbAddress);

await wallet.client.post(`/wallet/${wallet.id}/bid`, {
pinheadmz marked this conversation as resolved.
Show resolved Hide resolved
name: name,
bid: 0,
lockup: 1000
});
});

it('should fail to send bid with 0 value and 0 lockup', async () => {
await wallet.client.post(`/wallet/${wallet.id}/open`, {
name: name
});

await mineBlocks(treeInterval + 1, cbAddress);

const fn = async () => await wallet.client.post(`/wallet/${wallet.id}/bid`, {
name: name,
bid: 0,
lockup: 0
});

await assert.rejects(fn, {message: 'Output is dust.'});
pinheadmz marked this conversation as resolved.
Show resolved Hide resolved
});

it('should get all bids (single player)', async () => {
Expand Down