Review comments

[tjulien-ledger]

Reading more data than the variable can hold. These occur in multiple occasions:

• https://github.com/NestedFi/nested-ledger-plugin/blob/916afee2947dbc19a816e9538a4da275163b9020/src/handle_provide_parameter.c#L55
• https://github.com/NestedFi/nested-ledger-plugin/blob/916afee2947dbc19a816e9538a4da275163b9020/src/handle_provide_parameter.c#L115
• https://github.com/NestedFi/nested-ledger-plugin/blob/916afee2947dbc19a816e9538a4da275163b9020/src/handle_provide_parameter.c#L142-L143
• https://github.com/NestedFi/nested-ledger-plugin/blob/916afee2947dbc19a816e9538a4da275163b9020/src/handle_provide_parameter_structs.c#L110
• https://github.com/NestedFi/nested-ledger-plugin/blob/916afee2947dbc19a816e9538a4da275163b9020/src/handle_provide_parameter_structs.c#L128-L130
• https://github.com/NestedFi/nested-ledger-plugin/blob/916afee2947dbc19a816e9538a4da275163b9020/src/handle_provide_parameter_structs.c#L142
• https://github.com/NestedFi/nested-ledger-plugin/blob/916afee2947dbc19a816e9538a4da275163b9020/src/handle_provide_parameter_structs.c#L190-L192

• https://github.com/NestedFi/nested-ledger-plugin/blob/916afee2947dbc19a816e9538a4da275163b9020/src/handle_provide_parameter_structs.c#L204

  It is possible to use U2BE in most cases.

These are never used/needed, please remove them:

• https://github.com/NestedFi/nested-ledger-plugin/blob/916afee2947dbc19a816e9538a4da275163b9020/src/nested_plugin.h#L144-L148
• https://github.com/NestedFi/nested-ledger-plugin/blob/916afee2947dbc19a816e9538a4da275163b9020/src/handle_provide_parameter_structs.c#L105-L106
• https://github.com/NestedFi/nested-ledger-plugin/blob/916afee2947dbc19a816e9538a4da275163b9020/src/handle_provide_parameter_structs.c#L185-L186

Avoid uint16_t overflows. It is possible to set these variables to zero and when the next parameter is parsed they are decremented which results in overflows. Check these conditions and return error if they are set to zero. There is not a real security impact here but it should be fixed. The code is context->current_length--;

This comment can be removed: https://github.com/NestedFi/nested-ledger-plugin/blob/916afee2947dbc19a816e9538a4da275163b9020/src/handle_query_contract_ui.c#L9

What is the point of the following lines? https://github.com/NestedFi/nested-ledger-plugin/blob/916afee2947dbc19a816e9538a4da275163b9020/src/handle_init_contract.c#L48 https://github.com/NestedFi/nested-ledger-plugin/blob/916afee2947dbc19a816e9538a4da275163b9020/src/handle_provide_parameter_structs.c#L171

if none, please remove it.

To improved readability: While parsing the parameters the next_param is incremented at the final of switch statements, yet sometimes it is not necessary to do it so there are return statements inside it. To avoid breaking the flow of these switch statements set manually what the next_param should be (example: context->next_param = BIO__OFFSET_ARRAY_ORDERS;). This way we avoid having returns in the middle of switch statements.

The directory icons contains the boilerplate gifs, please replace them.

In the handle_destroy function https://github.com/NestedFi/nested-ledger-plugin/blob/916afee2947dbc19a816e9538a4da275163b9020/src/handle_provide_parameter.c#L113-L123

the read the number of tokens and it seems that it is assumed that the number_of_tokens will be equal to 1. Since when the next parameter is sent the context is set to analyse a order structure. What should be done is adding something like https://github.com/NestedFi/nested-ledger-plugin/blob/916afee2947dbc19a816e9538a4da275163b9020/src/handle_provide_parameter.c#L55-L62

Only when current_length == 0 the context should be set to analyse the order

Lastly, the lint action is missing

[adrienlacombe]

Other comment from the Donjon: Every time we read the length of an array from a parameter we need to make sure that the rest of the parameter bytes are actually zero. For instance https://github.com/NestedFi/nested-ledger-plugin/blob/916afee2947dbc19a816e9538a4da275163b9020/src/handle_provide_parameter.c#L55 we could craft a transaction that has a huge length such as 0x100000001 and the code will actually read the length as 1 which can misguide the user. Create a separate function that reads the length of the array from the parameter and check if the remaining bytes of the parameter are zero, if they are not set an error and continue execution normally as the app-ethereum will abort the signing process automatically (the function should return the length in any case)

[adrienlacombe]

@gpiriou can you please use U4BE_from_parameter as explained in https://github.com/LedgerHQ/app-ethereum/blob/develop/doc/ethapp_plugins.adoc#eth_plugin_provide_parameter ? Thank you!

[gpiriou]

We're on it ! 👌

Le lun. 12 déc. 2022 à 11:44, Adrien Lacombe @.***> a écrit :

@gpiriou https://github.com/gpiriou can you please use U4BE_from_parameter as explained in https://github.com/LedgerHQ/app-ethereum/blob/develop/doc/ethapp_plugins.adoc#eth_plugin_provide_parameter ? Thank you!

— Reply to this email directly, view it on GitHub https://github.com/NestedFi/nested-ledger-plugin/issues/4#issuecomment-1346255871, or unsubscribe https://github.com/notifications/unsubscribe-auth/AOJ4IL34AUXUZFRZ72BZVC3WM36XFANCNFSM6AAAAAAQY3V7KI . You are receiving this because you were mentioned.Message ID: @.***>

[Benjyskan]

We now use U4BE_from_parameter, but the CI fail on compilation, it seems to be due to U4BE_from_parameter’s commit not being merged to plugin-sdk’s master yet. Should we track plugin-sdk’s develop branch ?

[adrienlacombe]

yes @Benjyskan switch to develop

[Benjyskan]

@adrienlacombe-ledger should be good now

[adrienlacombe]

@Benjyskan @gpiriou some more comments below

• Move https://github.com/NestedFi/nested-ledger-plugin/blob/cf08c6ae3522ed120237b7b1b4d13d69038d9ea1/src/main.c#L28-L44 into a separate file called contract.c

• When BIO/BOO[] has len = 0 we should return an error, for instance https://github.com/NestedFi/nested-ledger-plugin/blob/cf08c6ae3522ed120237b7b1b4d13d69038d9ea1/src/handle_provide_parameter.c#L49. In other cases like https://github.com/NestedFi/nested-ledger-plugin/blob/cf08c6ae3522ed120237b7b1b4d13d69038d9ea1/src/handle_provide_parameter.c#L171, https://github.com/NestedFi/nested-ledger-plugin/blob/cf08c6ae3522ed120237b7b1b4d13d69038d9ea1/src/handle_provide_parameter_structs.c#L117, https://github.com/NestedFi/nested-ledger-plugin/blob/cf08c6ae3522ed120237b7b1b4d13d69038d9ea1/src/handle_provide_parameter_structs.c#L140, https://github.com/NestedFi/nested-ledger-plugin/blob/cf08c6ae3522ed120237b7b1b4d13d69038d9ea1/src/handle_provide_parameter_structs.c#L226 the same approach should be taken.

• Remove the following comment https://github.com/NestedFi/nested-ledger-plugin/blob/cf08c6ae3522ed120237b7b1b4d13d69038d9ea1/src/handle_provide_parameter.c#L134

• In https://github.com/NestedFi/nested-ledger-plugin/blob/cf08c6ae3522ed120237b7b1b4d13d69038d9ea1/src/handle_provide_parameter.c#L145-L149 we consider the case where context->number_of_tokens=0 and we always parse Order[0], while on parse_batched_input_orders for instance, we don’t take that into consideration and we always parse Order[-1] (last element), why is that?

• In https://github.com/NestedFi/nested-ledger-plugin/blob/cf08c6ae3522ed120237b7b1b4d13d69038d9ea1/src/handle_provide_parameter.c#L191 and in https://github.com/NestedFi/nested-ledger-plugin/blob/cf08c6ae3522ed120237b7b1b4d13d69038d9ea1/src/handle_provide_parameter.c#L209 it could be possible to set the next parameter as an error since no more parameters are expected.

• The lines https://github.com/NestedFi/nested-ledger-plugin/blob/cf08c6ae3522ed120237b7b1b4d13d69038d9ea1/src/handle_provide_parameter_structs.c#L28-L31 don’t seem to be necessary as they are not used. The same happens in https://github.com/NestedFi/nested-ledger-plugin/blob/cf08c6ae3522ed120237b7b1b4d13d69038d9ea1/src/handle_provide_parameter_structs.c#L95-L99 where context->current_tuple_offset is set and is later overwritten in https://github.com/NestedFi/nested-ledger-plugin/blob/cf08c6ae3522ed120237b7b1b4d13d69038d9ea1/src/handle_provide_parameter_structs.c#L150-L155 without being read (the same pattern occurs in https://github.com/NestedFi/nested-ledger-plugin/blob/cf08c6ae3522ed120237b7b1b4d13d69038d9ea1/src/handle_provide_parameter_structs.c#L201)

• It seems that when the selected method is create/processInputOrders/processOutputOrders until the last Order is reached we parse other unnecessary Orders https://github.com/NestedFi/nested-ledger-plugin/blob/cf08c6ae3522ed120237b7b1b4d13d69038d9ea1/src/handle_provide_parameter_structs.c#L21-L25. It should only be parsing the wanted Order.

• It seems that if token2_address is only set if len(Order[]) == 1, therefore if len(Order[] != 1) it is never set and it is used for instance in https://github.com/NestedFi/nested-ledger-plugin/blob/cf08c6ae3522ed120237b7b1b4d13d69038d9ea1/src/text_utils.c#L27

• The code in https://github.com/NestedFi/nested-ledger-plugin/blob/cf08c6ae3522ed120237b7b1b4d13d69038d9ea1/src/handle_provide_parameter_structs.c#L127-L136 could probably simplified to :

  if (context->current_length) context->current_length--;
  // skip until last amount
  if (context->current_length == 0){
    copy_parameter(context->token1_amount, msg->parameter, sizeof(context->token1_amount));
    PRINTF("copie token1 amount: %.*H\n", PARAMETER_LENGTH, context->token1_amount);
    context->next_param = (batch_output_orders) BOO__LEN_ORDERS;
  }
  break;

• There are a few tests that only target nanos (should also target nanox/nanosp)

[gpiriou]

we always parse Order[-1] (last element), why is that?

• We use -1 to match b2c expected element, and we need to know where is the last bytes of the entire TX, as we have little memory available, to kill 2 birds with 1 stone.

It seems that if token2_address is only set if len(Order[]) == 1, therefore if len(Order[] != 1) it is never set and it is used for instance in

• token2_address is only used in certain cases on Nested front (swap, sell tokens, withdraw), in all these cases last Order[] Len will be 1. We added an error anyway to prevent unwanted display.

[adrienlacombe]

ty @gpiriou some more (small) comments

• The comment should be regarding amount[] https://github.com/NestedFi/nested-ledger-plugin/blob/98a9238af7154359bfe291884a860647a73e8669/src/handle_provide_parameter_structs.c#L107

• The comment should be regarding Order[] https://github.com/NestedFi/nested-ledger-plugin/blob/98a9238af7154359bfe291884a860647a73e8669/src/handle_provide_parameter_structs.c#L129

• Is there any documentation for this? /* ui_selector is the byte set by Nested frontend to determine the action put in screen ID */

[Benjyskan]

Thanks for the quick response, we've updated master branch.

[adrienlacombe]

small notes @gpiriou @Benjyskan

• On processInputOrders if len(Order[]) > 1 and the ui_selector == SELL/SWAP, the token2_address is never set by the plugin, therefore it has the value 0. So in handle_sell_tokens_ui/handle_swap_ui on screenIndex == 1, the function msg_ticker_or_address(2) will use the token2_ticker which is incorrect.

• On processOutputOrders if len(Order[]) > 1 and the ui_selector == SWAP, the token1_address is not initialized, then on handle_swap_ui it is used without checking if it is actually initialized.

• On https://github.com/NestedFi/nested-ledger-plugin/blob/cf08c6ae3522ed120237b7b1b4d13d69038d9ea1/src/handle_provide_parameter.c#L145-L149 why the Order[0] is parsed instead of Order[-1] like in create/processInputOrders/processOutputOrders

[gpiriou]

Q: On processInputOrders if len(Order[]) > 1 and the ui_selector == SELL/SWAP, the token2_address is never set by the plugin, therefore it has the value 0. So in handle_sell_tokens_ui/handle_swap_ui on screenIndex == 1, the function msg_ticker_or_address(2) will use the token2_ticker which is incorrect.

R: for SELL, if len(Order[]) > 1 then number_of_tokens > 1 and TOKEN2_FOUND is false so: it should never display token2_ticker nor token2_address. We've added a check in finalize that will crash the plugin if the selector is SWAP and number_of_tokens > 1, Nested front should never create such transactions as far as we know.

Q: On processOutputOrders if len(Order[]) > 1 and the ui_selector == SWAP, the token1_address is not initialized, then on handle_swap_ui it is used without checking if it is actually initialized.

R: The check handles this as well.

Q: (destroy) why the Order[0] is parsed instead of Order[-1] like in create/processInputOrders/processOutputOrders ?

R: As we only parse the order if there is 1 order, it changes nothing between [0] and [-1]. We still changed it to [-1] for consistency.