RocketMinipoolDelegate - Sandwiching of Minipool calls can have unintended side effects
The `RocketMinipoolBase` contract exposes the functions `delegateUpgrade` and `delegateRollback`, allowing the minipool owner to switch between delegate implementations. While giving the minipool owner a chance to roll back potentially malfunctioning upgrades, the fact that upgrades and rollback are instantaneous also gives them a chance to alternate between executing old and new code (e.g. by utilizing callbacks) and sandwich user calls to the minipool.\\nAssuming the latest minipool delegate implementation, any user can call `RocketMinipoolDelegate.slash`, which slashes the node operator's RPL balance if a slashing has been recorded on their validator. To mark the minipool as having been `slashed`, the `slashed` contract variable is set to `true`. A minipool owner can avoid this flag from being set By sandwiching the user calls:\\nIn detail, the new slash implementation:\\n```\\nfunction \\_slash() private {\\n    // Get contracts\\n    RocketNodeStakingInterface rocketNodeStaking = RocketNodeStakingInterface(getContractAddress("rocketNodeStaking"));\\n    // Slash required amount and reset storage value\\n    uint256 slashAmount = nodeSlashBalance;\\n    nodeSlashBalance = 0;\\n    rocketNodeStaking.slashRPL(nodeAddress, slashAmount);\\n    // Record slashing\\n    slashed = true;\\n}\\n```\\n\\nCompared to the old slash implementation:\\n```\\nfunction \\_slash() private {\\n    // Get contracts\\n    RocketNodeStakingInterface rocketNodeStaking = RocketNodeStakingInterface(getContractAddress("rocketNodeStaking"));\\n    // Slash required amount and reset storage value\\n    uint256 slashAmount = nodeSlashBalance;\\n    nodeSlashBalance = 0;\\n    rocketNodeStaking.slashRPL(nodeAddress, slashAmount);\\n}\\n```\\n\\nWhile the bypass of `slashed` being set is a benign example, the effects of this issue, in general, could result in a significant disruption of minipool operations and potentially affect the system's funds. The impact highly depends on the changes introduced by future minipool upgrades.
We recommend limiting upgrades and rollbacks to prevent minipool owners from switching implementations with an immediate effect. A time lock can fulfill this purpose when a minipool owner announces an upgrade to be done at a specific block. A warning can precede user-made calls that an upgrade is pending, and their interaction can have unintended side effects.
```\\nfunction \\_slash() private {\\n    // Get contracts\\n    RocketNodeStakingInterface rocketNodeStaking = RocketNodeStakingInterface(getContractAddress("rocketNodeStaking"));\\n    // Slash required amount and reset storage value\\n    uint256 slashAmount = nodeSlashBalance;\\n    nodeSlashBalance = 0;\\n    rocketNodeStaking.slashRPL(nodeAddress, slashAmount);\\n    // Record slashing\\n    slashed = true;\\n}\\n```\\n