Double-precision math should be avoided in consensus - (nft_lottery_token_purchase) #421

New issue

Closed

opened 2022-08-29 16:49:09 +00:00 by christophersanborn · 9 comments

christophersanborn commented

2022-08-29 16:49:09 +00:00

(Migrated from gitlab.com)

To avoid difficult-to-anticipate edge cases, consensus logic should avoid floating-point math.

A search turns up a handful of uses of floating-point match for consensus logic. One in particular is in the nft_lottery_token_purchase_evaluator:

nft_lottery_evaluator.cpp#L33:

    auto lottery_options = lottery_md_obj.lottery_data->lottery_options;
    FC_ASSERT(lottery_options.ticket_price.asset_id == op.amount.asset_id);
    FC_ASSERT((double)op.amount.amount.value / lottery_options.ticket_price.amount.value == (double)op.tickets_to_buy);

Here it is used to check that the amount of asset provided in the ticket purchase operation is correct for the purchase of tickets_to_buy NFT lotto tickets.

Recommendation:

The check should be replaced by a two-step check that:

(1) checks equivalency using integer multiplication (instead of division), and
(2) checks for overflow.

Instead of:

    FC_ASSERT((double)op.amount.amount.value / lottery_options.ticket_price.amount.value == (double)op.tickets_to_buy);

Recommend:

    share_type tickets_to_buy(op.tickets_to_buy);  // (cast to safe<int64>)
    FC_ASSERT(tickets_to_buy >= 0);  // (catch overflow in uint64 -> int64)
    FC_ASSERT(tickets_to_buy * lottery_options.ticket_price.amount.value == op.amount.amount.value);

This actually achieves BOTH objectives, due to bounds-checking already in share_type.

Overflow check:

Note that share_type is typedef'd in types.hpp as:

   typedef safe<int64_t>           share_type;

And that multiplication of two share_type's is therefore already overloaded to check for overflows. (see: safe.hpp)

However, note that op.tickets_to_buy is not already a share_type, but rather a uint64. Casting it to share_type should effectively turn on the bounds checking. But note also that we will need to reject values of op.tickets_to_buy with the high-bit set because they will wrap-around and become negative. This means rejecting a value which is allowed (albeit erroneously, imho) by protocol. However in practice this is not a problem because ticket purchases this large would not be realizable because they exceed the number of object IDs available for NFT objects. So it should be OK to reject these.

Tests:

The test suite should include a test that confirms rejection of a purchase operation that overflows.

Additional Suggestion:

To avoid the awkwardness of having to cast op.tickets_to_buy to a share_type, it would be better if nft_lottery_token_purchase_operation::tickets_to_buy were already share_type instead of uint64_t. (See definition in nft_lottery.hpp#L21.)

In general, protocol should NOT be modified after deployment, but in this case it may be worthwhile, if done carefully. IMHO a separate ticket should be created for this revision.

Suggestion would be to change the type from uint64_t to share_type and ensure validation rejects negative values and values exceeding the number of available object IDs.

To avoid difficult-to-anticipate edge cases, consensus logic should avoid floating-point math. A search turns up a handful of uses of floating-point match for consensus logic. One in particular is in the nft_lottery_token_purchase_evaluator: [nft_lottery_evaluator.cpp#L33](https://gitlab.com/PBSA/peerplays/-/blob/develop/libraries/chain/nft_lottery_evaluator.cpp#L33): ```cpp auto lottery_options = lottery_md_obj.lottery_data->lottery_options; FC_ASSERT(lottery_options.ticket_price.asset_id == op.amount.asset_id); FC_ASSERT((double)op.amount.amount.value / lottery_options.ticket_price.amount.value == (double)op.tickets_to_buy); ``` Here it is used to check that the amount of asset provided in the ticket purchase operation is correct for the purchase of `tickets_to_buy` NFT lotto tickets. ### Recommendation: The check should be replaced by a two-step check that: - (1) checks equivalency using integer multiplication (instead of division), and - (2) checks for overflow. Instead of: ```cpp FC_ASSERT((double)op.amount.amount.value / lottery_options.ticket_price.amount.value == (double)op.tickets_to_buy); ``` Recommend: ```cpp share_type tickets_to_buy(op.tickets_to_buy); // (cast to safe<int64>) FC_ASSERT(tickets_to_buy >= 0); // (catch overflow in uint64 -> int64) FC_ASSERT(tickets_to_buy * lottery_options.ticket_price.amount.value == op.amount.amount.value); ``` This actually achieves BOTH objectives, due to bounds-checking already in `share_type`. #### Overflow check: Note that `share_type` is typedef'd in [types.hpp](https://gitlab.com/PBSA/peerplays/-/blob/develop/libraries/chain/include/graphene/chain/protocol/types.hpp#L370) as: ```cpp typedef safe<int64_t> share_type; ``` And that multiplication of two `share_type`'s is therefore already overloaded to check for overflows. (see: [safe.hpp](https://gitlab.com/PBSA/tools-libs/peerplays-fc/-/blob/156b0c4e41c9215eadb2af8009b05e0f38c16dda/include/fc/safe.hpp#L52)) However, note that `op.tickets_to_buy` is not already a `share_type`, but rather a `uint64`. Casting it to `share_type` should effectively turn on the bounds checking. But note also that we will need to reject values of `op.tickets_to_buy` with the high-bit set because they will wrap-around and become negative. This means rejecting a value which is allowed (albeit erroneously, imho) by protocol. However in practice this is not a problem because ticket purchases this large would not be realizable because they exceed the number of object IDs available for NFT objects. So it should be OK to reject these. ### Tests: The test suite should include a test that confirms rejection of a purchase operation that overflows. ### Additional Suggestion: To avoid the awkwardness of having to cast `op.tickets_to_buy` to a `share_type`, it would be better if `nft_lottery_token_purchase_operation::tickets_to_buy` were already `share_type` instead of `uint64_t`. (See definition in [nft_lottery.hpp#L21](https://gitlab.com/PBSA/peerplays/-/blob/develop/libraries/chain/include/graphene/chain/protocol/nft_lottery.hpp#L21).) In general, protocol should NOT be modified after deployment, but in this case it may be worthwhile, if done carefully. IMHO a separate ticket should be created for this revision. Suggestion would be to change the type from `uint64_t` to `share_type` and ensure validation rejects negative values and values exceeding the number of available object IDs.

serkixenos commented

2022-10-11 00:14:19 +00:00

(Migrated from gitlab.com)

assigned to @mkhan17

serkixenos commented

2022-10-14 18:44:09 +00:00

(Migrated from gitlab.com)

assigned to @milo_peerplays

milo_peerplays commented

2023-02-07 12:45:19 +00:00

(Migrated from gitlab.com)

mentioned in merge request !205