Closed mihalacherazvan closed 2 years ago
Ahh, good catch. This is a bug. If a memo is provided, we'll need to check for the transfer in the second index.
This is a spec violation -- https://github.com/solana-labs/solana-pay/blob/master/SPEC.md#memo
The spec requires the the memo come immediately before the transfer, but we are checking for it in the second index:
Can you build and try out #148?
The only issue i see here is that you are relying on the person writing the validation logic to actually pass the memo
field to the validateTransfer
function. I, personally, am not doing this in neither my test nor my live setup because i was under the impression that checking with reference
, amount
, recipient
and optionally splToken
is enough to guarantee the identification (and validation) of the correct transaction, even if it also had a Memo instruction. Therefore, my test also failed with these changes. What are your thoughts on this ?
Thanks!
The only issue i see here is that you are relying on the person writing the validation logic to actually pass the
memo
field to thevalidateTransfer
function.
The idea is that you should use the same fields for validateTransfer
as the wallet would use for createTransfer
which should be the same as what's used in encodeURL
, with the exception being that the amount
is optional in the URL but required in the transaction and its validation (and label
and message
aren't part of the transaction, so therefore cannot be validated).
checking with
reference
,amount
,recipient
and optionallysplToken
is enough to guarantee the identification (and validation) of the correct transaction
"Enough" is debatable, because memo
can be used for validation as well (and is used for this, by some apps) because you might use the memo to locate or direct the funds after transfer. This was always intended, but it's a shortcoming of the SDK that we didn't implement this until ac6ce0d0a81137700874a8bf5a7caac3be999fad:
https://github.com/solana-labs/solana-pay/blob/f41701dc7931f7882c6eb0582c9ddd796eb9d3aa/core/src/validateTransfer.ts#L78
So I think there are a few questions for us to tackle --
A way we could make this looser is to look for a memo instruction regardless of whether memo
is provided, and allow it. We need to make sure this doesn't screw with reference keys though -- you shouldn't be able to use a reference key on the memo
instruction at all, which is perhaps another check we should add.
A way we could make this looser is to look for a memo instruction regardless of whether
memo
is provided, and allow it.
Personally, i would incline for this approach, but i also understand the idea that all parameters used in the creation of the transaction should be used when verifying it.
Do you think that the looser approach could be implemented somewhat like this ?
if (transaction.instructions[instructionIndex].programId.equals(MEMO_PROGRAM_ID)) {
instructionIndex++;
if (memo !== undefined) {
validateMemo(transaction.instructions[instructionIndex], memo);
}
}
to replace this if
statement: https://github.com/solana-labs/solana-pay/blob/fix-147/core/src/validateTransfer.ts#L78
We need to make sure this doesn't screw with reference keys though -- you shouldn't be able to use a reference key on the
memo
instruction at all, which is perhaps another check we should add.
I understand that reference keys and memo fields serve different purposes, but what undesired effect do you think using a reference key on the memo
instruction would have ? And by this do you mean actually storing the reference key inside the memo field or something else ?
PS, sorry for the long delay in my responses, but i'm on the UTC+3 time zone.
Reference keys should only be added as accounts to the transfer instruction. Making sure none are added to the memo instruction prevents manipulation of the location / validation logic by a malicious party, which is what ac6ce0d0a81137700874a8bf5a7caac3be999fad was created to fix. I don't think this is an attack vector now, just need to make sure.
I changed how this works, which I think should be nonbreaking -- https://github.com/solana-labs/solana-pay/blob/1218e8272365f5937248657697aefdfff4c8cdec/core/src/validateTransfer.ts#L74-L98
We check that the last instruction is the transfer instruction, and that the previous one is the memo (if provided). There was someone else who was interested in putting some instructions at the beginning of the tx, so this also solves their issue.
I merged this but will wait to release it. Can you tell me if your previous tests pass? And let me know if you have any other feedback.
Hello! With your latest changes my tests work fine so as far as i'm concerned everything is ok now. Thank you!
Hello!
When using the
validateTransfer
method with bothreference
andsplToken
values provided in theValidateTransferFields
parameter to validate a transaction that also had an instruction for the Memo program, the Memo instruction will be passed first in the array of instructions for that transaction and the following line results in aTokenInvalidInstructionDataError
exception being thrown:https://github.com/solana-labs/solana-pay/commit/ac6ce0d0a81137700874a8bf5a7caac3be999fad#diff-c188b51c3b4a63c0d81bdb64891b7010f33dacfba2f29e900756ca3be8d35a2eR131
If i don't pass the
reference
value in theValidateTransferFields
parameter then thevalidateTransfer
function works as intended because theif (references) {...}
clause is skipped.Maybe there should be a check for which of the transaction's instructions is an SPL Token transfer instruction or whether the first instruction is a Memo instruction before passing it to the
decodeInstruction
method.Thank you!