Durable nonce transactions resulting in an instruction error allow fee theft

[t-nelson]

Problem

Solana typically charges fees on transactions which succeed or fail with an InstructionError. In the event of the latter, the state of every modified account is also rolled back to before execution. Since a durable nonce transaction advances the stored nonce with an instruction, if the transaction fails with an InstructionError, the old stored nonce is replaced. Due to durable nonces having arbitrary lifetimes, it is infeasible to maintain a signature blacklist against each nonce value, ala. StatusCache. As such, these failed transactions can be replayed and fees charged until the stored nonce value is successfully advanced.

Proposed Solution

The stored nonce MUST advance whenever the account state or balance changes, including paying fees. This can be achieved by always storing the updated nonce accounts.

Alternatively, the stored nonce can be advanced outside the program. If this path is chosen, moving the durable nonce feature into system program should be considered.

cc/ @rob-solana

[rob-solana]

How does advancing the Nonce outside the transaction work? Unless the Nonce is advanced in the same transaction as the Nonced operation, the Nonced operation can be replayed.

I think moving the Nonce into the system program should be considered in any case. We might even be able to avoid requiring a Nonce instruction, and instead say that "any time a Nonce is used as the fee account, the Nonce must advance".

[rob-solana]

we would also be able to do away with the nonce's authority field...

[t-nelson]

How does advancing the Nonce outside the transaction work? Unless the Nonce is advanced in the same transaction as the Nonced operation, the Nonced operation can be replayed.

Runtime will do the updating based on can_commit()'s rules. It seems to me that any fallible, introspective logic in the nonce instruction will lead to replayable fee theft.

I think moving the Nonce into the system program should be considered in any case. We might even be able to avoid requiring a Nonce instruction, and instead say that "any time a Nonce is used as the fee account, the Nonce must advance".

I think you're right. Are you thinking all system accounts will carry nonce data, or that we'll introduce a new species of system account?

[rob-solana]

new species of system account

[aeyakovenko]

@rob-solana @t-nelson why not just let it be. The transaction should succeed, and the user can guard against failure by using a low balance system account.

[t-nelson]

I'd rather not leave a footgun if at all possible. Have we received any feedback from partners regarding this issue and whether it's a concern?

[aeyakovenko]

@t-nelson that was the motivation for spending the spending key account fully in the original design. @garious's proposal is starting to feel way simpler.

[rob-solana]

@rob-solana @t-nelson why not just let it be. The transaction should succeed, and the user can guard against failure by using a low balance system account.

the main user is currently planning to have a non-trivial balance in the fee account

[aeyakovenko]

@rob-solana @t-nelson

Is the proposal basically:
If it’s system, then than to commit the fee the nonce update must be committed as well.

I guess I don’t hate it. Let’s do it.

[t-nelson]

You got it!

[rob-solana]

@t-nelson remember that transfers out of a system account (including paying the fee) with a Nonce need to have that "ensure cannot be re-created" check. The account has to stay rent exempt.

I think this is the only messy thing about pushing Nonce facilities into system_program, but at least all those checks end up in the same couple of files.

[t-nelson]

@rob-solana 10-4, they'll die with TransactionError::InsufficientFundsForFee.

I'm less clear on the least intrusive way to discern between system account species. Could go with flags or use the owner ID and tweak the system program checks. wdyt?

[rob-solana]

what's wrong with testing whether the data is present and an initialized Nonce?

transfer() (and other lamport movement) would need to verify that the Nonce is empty (data.len() == 0 || deserialize(data) == Some(UninitializedNonce)). moving funds out of a Nonce has to go through withdraw(), which is presented with sysvar::RecentBlockhashes (right?)

[aeyakovenko]

I think testing the data is fine. Since system controls this thing. One thing to add. System::Assign can only work on 0 data. Prior to assign we need to ensure the data is still 0, not a nonce account.

[t-nelson]

I suppose there's nothing wrong with it. I just wasn't thinking of deserialization succeeding as much of a guaranty. This seems reasonable so long as we don't go and pull the same trick with the same data layout later.

So we're looking at

1. Two new system instructions
   • CreateNonceAccount
   • WithdrawNonceAccount
2. data based gating of these existing system instructions
   • Transfer (and programmatic balance changes)
   • Assign
3. Advance nonce whenever fee payer is a nonce account
4. Store nonce account for every TX where Bank::can_commit() is true

[aeyakovenko]

@t-nelson only system will ever write to it. You can check if the first word in data is non-zero and deserialize. Just make sure assign checks that that it’s zero, otherwise we have an attack vector to leak state into programs :)

[t-nelson]

@rob-solana on my way to an implementation of this, I've run into some rent-related tests with fee-paying accounts carrying non-zero-sized, but small and unused, data segments. Since we charge rent on zero-data accounts as of #6888, can these accounts safely be zero-data now? Alternative being to add a separate fee-payer account, which will require plumbing through a few of the transaction constructors.

[rob-solana]

@t-nelson those test accounts can safely be made zero-data