Insufficient htlc_extend_operation validation

[abitmore]

Bug Description

1. Everyone can extend a HTLC, which means funds inside can be locked forever (although need to pay per-day fee).
   

   bitshares-core/libraries/chain/htlc_evaluator.cpp

   Lines 123 to 127 in 11432f3

   	void_result htlc_extend_evaluator::do_evaluate(const htlc_extend_operation& o)
   	{
   	htlc_obj = &db().get<htlc_object>(o.htlc_id);
   	return void_result();
   	}

2. Theoretically the sender can overflow a HTLC and got refunded (although may need to pay a large per-day fee).
   

   bitshares-core/libraries/chain/htlc_evaluator.cpp

   Lines 129 to 136 in 11432f3

   	void_result htlc_extend_evaluator::do_apply(const htlc_extend_operation& o)
   	{
   	db().modify(*htlc_obj, [&o](htlc_object& db_obj) {
   	db_obj.conditions.time_lock.expiration += o.seconds_to_add;
   	});
   	return void_result();
   	}

Impacts
Describe which portion(s) of BitShares Core may be impacted by this bug. Please tick at least one box.

• API (the application programming interface)
• Build (the build process or something prior to compiled code)
• CLI (the command line wallet)
• Deployment (the deployment process after building such as Docker, Travis, etc.)
• DEX (the Decentralized EXchange, market engine, etc.)
• P2P (the peer-to-peer network for transaction/block propagation)
• Performance (system or user efficiency, etc.)
• Protocol (the blockchain logic, consensus, validation, etc.)
• Security (the security of system or user data, etc.)
• UX (the User Experience)
• Other (please add below)

CORE TEAM TASK LIST

• Evaluate / Prioritize Bug Report
• Refine User Stories / Requirements
• Define Test Cases
• Design / Develop Solution
• Perform QA/Testing
• Update Documentation

[xeroc]

Related, the fee_payer for that operation:

bitshares-core/libraries/chain/include/graphene/chain/protocol/htlc.hpp

Lines 168 to 169 in 11432f3

	*/
	account_id_type fee_payer()const { return update_issuer; }

with no check if the update_issuer has to do with the HLTC, at all.

The question is whether this is a bug or a feature.

[pmconrad]

This is clearly a bug. BSIP-44 specifies checks on both update_issuer and time:

transaction_obj htlc_extend(id, issuer, timeout_threshold, broadcast)
Validate: 'issuer' = get_htlc(id).from
Validate: timeout_threshold < now() + GRAPHENE_HTLC_MAXIMUM_DURRATION

How the hell did we miss that?

[abitmore]

By the way, the fee_payer in htlc_redeem_operation is not validated as well, however the funds will be sent to the to account of the HTLC object, so it's not a big deal.

bitshares-core/libraries/chain/htlc_evaluator.cpp

Lines 100 to 121 in 11432f3

	void_result htlc_redeem_evaluator::do_evaluate(const htlc_redeem_operation& o)
	{
	htlc_obj = &db().get<htlc_object>(o.htlc_id);
	FC_ASSERT(o.preimage.size() == htlc_obj->conditions.hash_lock.preimage_size, "Preimage size mismatch.");
	const htlc_redeem_visitor vtor( o.preimage );
	FC_ASSERT( htlc_obj->conditions.hash_lock.preimage_hash.visit( vtor ), "Provided preimage does not generate correct hash.");
	return void_result();
	}
	void_result htlc_redeem_evaluator::do_apply(const htlc_redeem_operation& o)
	{
	db().adjust_balance(htlc_obj->transfer.to, asset(htlc_obj->transfer.amount, htlc_obj->transfer.asset_id) );
	// notify related parties
	htlc_redeemed_operation virt_op( htlc_obj->id, htlc_obj->transfer.from, htlc_obj->transfer.to,
	asset(htlc_obj->transfer.amount, htlc_obj->transfer.asset_id ) );
	db().push_applied_operation( virt_op );
	db().remove(*htlc_obj);
	return void_result();
	}

An issue related to this is:

bitshares-core/libraries/chain/db_notify.cpp

Lines 265 to 272 in 11432f3

	void operator()( const htlc_redeem_operation& op )
	{
	_impacted.insert( op.fee_payer() );
	}
	void operator()( const htlc_redeemed_operation& op )
	{
	_impacted.insert( op.from );
	}

which means the real to account won't be notified when the redemption is done by a 3rd party.

[jmjatlanta]

As for htlc_redeem: I cannot think of a scenario where it would be advantageous to allow someone else to execute an HTLC. I think we should validate that the redeemer is the "from".

If someone can think of a scenario, then we can conditionally add "to" to the _impacted collection.

I do not feel this redeem issue is urgent. I await the opinions of others.

[abitmore]

@jmjatlanta in the scenario: A initialized a HTLC to transfer to B, C redeemed, get_account_history of A contains a create and a redeemed, history of B contains only a create, history of C contains only a redeem. It would be a bit strange that B doesn't have a history entry while the balance increased.

Actually, if we can prevent another account from redeeming the HTLC, this issue would be gone. That said, we should validate that the redeemer is the "to".

[jmjatlanta]

Actually, if we can prevent another account from redeeming the HTLC, this issue would be gone. That said, we should validate that the redeemer is the "to".

Done as part of #1699. Please take a look.

[pmconrad]

https://github.com/bitshares/bsips/blob/master/bsip-0044.md#redeem specifies that

// NOTE: any account may attempt to redeem

IMO the correct fix is to include htlc_redeemed_operation also in the history of to.

[jmjatlanta]

IMO the correct fix is to include htlc_redeemed_operation also in the history of to.

I guess we should follow the spec. #1699 updated.

[abitmore]

I agree that it's good to follow the spec. However, it means the history object numbering of new code is different than 3.0.0, thus we need to force everyone to upgrade to avoid potential data mismatching issues (which would be messy). So IMHO under the current situation it's easier and more practicable to apply a more strict rule (can be witness-only) even if to is added to impacted of htlc_redeemed_operation until next consensus release.

[abitmore]

Fixed by #1699.