tlv-account-resolution: Always demote signer flag#164
tlv-account-resolution: Always demote signer flag#164joncinque merged 2 commits intosolana-program:mainfrom
Conversation
#### Problem As described at solana-program/transfer-hook#83, there's just too many ways for signers to be potentially abused during transfer hooks. #### Summary of changes Demote all accounts to non-signer when resolving from an extra account metas list.
buffalojoec
left a comment
buffalojoec
left a comment
There was a problem hiding this comment.
Lgtm. This is definitely the safer route to take.
tlv-account-resolution/src/state.rs
Outdated
| account_info_to_meta(&account_infos[0]), | ||
| account_info_to_meta(&account_infos[1]), | ||
| account_info_to_meta(&account_infos[2]), | ||
| de_escalate_signer(account_info_to_meta(&account_infos[0])), |
There was a problem hiding this comment.
Can't you just use account_info_to_meta_non_signer for all of these?
There was a problem hiding this comment.
Ah yes, nice catch!
There was a problem hiding this comment.
Well it's only possible on these three, the rest are account metas already, not account infos
tlv-account-resolution/src/state.rs
Outdated
| // This is a little tricky to read, but the idea is to see if this account | ||
| // is marked as writable anywhere in the instruction at the start. If so, | ||
| // DON'T escalate it to be a writer in the CPI |
There was a problem hiding this comment.
Is this comment incorrect? Shouldn't it say:
Check to see if the account is writable in the original instruction...
- "If it's not, don't escalate it"
- or: "If it's not, deescalate the extra meta
is_writableconfig"
But just worded better than what I wrote.
There was a problem hiding this comment.
Yeah that makes more sense, thanks!
|
Just checking - when the program is upgraded to deescalate any signers in the Any particular plan to this upgrade and breaking behavioral change? |
From what I understand, there aren't really any uses of signers in transfer hooks, but we'll need to work with Foundation eng to get the word out -- @tiago18c can you help with that? |
|
I did some digging:
Basically there will be no one affected (other than possible black hats). |
3 participants
Problem
As described at solana-program/transfer-hook#83, there's just too many ways for signers to be potentially abused during transfer hooks.
Summary of changes
Demote all accounts to non-signer when resolving from an extra account metas list.