Sequencer suffers from TOCTOU issues

[SuperFluffy]

Bug found by auditors:

Due to the way action handling is done, unexpected behaviors can be noted.

a transactions action's stateful checks are ran together in parallel. Then the actions are executed.

Action 1 -> check_stateful()
Action 2 -> check_stateful()
...
Action 1 -> Execute()
Action 2 -> Execute()

This means that certain actions could have unforseen consequences. Imagine 2 BridgeSudoChangeAction actions which have been bundled into 1 tx, they would both execute even if the first action changes the sudo address because the stateful checks of both actions are executed before the execute

    async fn check_stateless(&self) -> anyhow::Result<()> {
        ensure!(!self.actions.is_empty(), "must have at least one action");

        for action in &self.actions {
            match action {

┆Issue Number: ENG-666

[SuperFluffy]

Some thoughts I have for this:

Because check_stateless, check_stateful, and execute are split and run one after the other, this inherently creates ace conditions. The way to fix this is to move the checks into execute. Given their serial nature this addresses the problem.

Looking at the current state of cnidarium, I find that they changed their naming in cndiarium_component::ActionHandler to the following:

• check_stateless (as before)
• check_historical (where they explicitly highlight the issue we are facing: time-of-check-to-time-of-use: https://github.com/penumbra-zone/penumbra/blob/6981aec13519d352be642f690350b5227657de4f/crates/cnidarium-component/src/action_handler.rs#L56-L59)
• check_and_execute (where now stateful check && execution happen atomically, taking into account serial execution)