Audit fixes #32
Audit fixes #32
Conversation
| if (cla != CLA) { | ||
| THROW(APDU_CODE_COMMAND_NOT_ALLOWED); | ||
| } |
There was a problem hiding this comment.
Why are we doing this so explicit when we don't do the same on other apps? any difference?
There was a problem hiding this comment.
This change is discussible, and I'm glad you point it out.
There might be a case where the transaction has ETH_CLA, and it comes with an ETH instruction not supported but that collides with a flare app instruction.
The app would treat this ETH transaction has a flare transaction.
The format of the transactions is different, so I would say it's impossible the app would parse and sign with success an ETH transaction using the flare parser.
But the auditors pointed this out, so I decided to add that extra check to make sure only flare transactions go through.
What do you think @emmanuelm41 ?
| @@ -118,8 +118,7 @@ zxerr_t _sign(uint8_t *output, uint16_t outputLen, const uint8_t *message, uint1 | |||
| const err_convert_e err_c = | |||
| convertDERtoRSV(signature->der_signature, tmpInfo, signature->r, signature->s, &signature->v); | |||
| if (err_c == no_error) { | |||
| *sigSize = | |||
| sizeof_field(signature_t, r) + sizeof_field(signature_t, s) + sizeof_field(signature_t, v) + signatureLength; | |||
There was a problem hiding this comment.
Was this signaatureLength empty (0) before? or was not this working?
There was a problem hiding this comment.
In the rypto-sign for non-evm transactions, we are only returning the signature in the r,s,v format.
Here we should be doing the same because the DER format was not needed.
No description provided.