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

lightningd: don't allow zero cltv HTLCs. #2214

Merged
merged 1 commit into from Jan 4, 2019

Conversation

Projects
None yet
2 participants
@rustyrussell
Copy link
Contributor

rustyrussell commented Jan 3, 2019

Seems like someone is sending out zero-cltv HTLCs, which is we weren't catching correctly, thus failing on a sanity check.

lightningd: don't allow zero cltv HTLCs.
Fixes: #2077
Fixes: #2213
Signed-off-by: Rusty Russell <rusty@rustcorp.com.au>
@cdecker

This comment has been minimized.

Copy link
Member

cdecker commented Jan 3, 2019

Shouldn't this check have caught that exact problem?

if (get_block_height(ld->topology) >= outgoing_cltv_value) {
log_debug(hin->key.channel->log,
"Expiry cltv %u too close to current %u",
outgoing_cltv_value,
get_block_height(ld->topology));
failcode = WIRE_EXPIRY_TOO_SOON;
goto fail;
}

I don't see how a 0 CLTV could get past that check.

@cdecker

This comment has been minimized.

Copy link
Member

cdecker commented Jan 3, 2019

CI failure was a flake btw, re-running.

@rustyrussell

This comment has been minimized.

Copy link
Contributor Author

rustyrussell commented Jan 3, 2019

Shouldn't this check have caught that exact problem?

lightning/lightningd/peer_htlcs.c

Lines 524 to 531 in 0321d79
if (get_block_height(ld->topology) >= outgoing_cltv_value) {
log_debug(hin->key.channel->log,
"Expiry cltv %u too close to current %u",
outgoing_cltv_value,
get_block_height(ld->topology));
failcode = WIRE_EXPIRY_TOO_SOON;
goto fail;
}

I don't see how a 0 CLTV could get past that check.

That's examining the CLTV inside the onion, not the incoming CLTV value.

What seems to be happening is that the onion is valid, but the previous peer decided to offer an HTLC with ctlv of 0.

@cdecker

This comment has been minimized.

Copy link
Member

cdecker commented Jan 4, 2019

Oh, that makes sense, my head was hurting a bit with the deltas and the various CLTVs :-)

ACK de25692

@cdecker cdecker merged commit 3006844 into ElementsProject:master Jan 4, 2019

2 checks passed

ackbot PR ack'd by cdecker
continuous-integration/travis-ci/pr The Travis CI build passed
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment