28777 - [SC - Low] pufETHsrcTimelocksolexecuteTransaction - This b...
pufETH/src/Timelock.sol::executeTransaction()
- This bug makes it possible to unexpectedly execute a timelocked queued transaction TWICE, accidentally/mistakenly.
pufETH/src/Timelock.sol::executeTransaction()
- This bug makes it possible to unexpectedly execute a timelocked queued transaction TWICE, accidentally/mistakenly.Submitted on Feb 26th 2024 at 21:41:35 UTC by @OxSCSamurai for Boost | Puffer Finance
Report ID: #28777
Report type: Smart Contract
Report severity: Low
Target: https://etherscan.io/address/0x3C28B7c7Ba1A1f55c9Ce66b263B33B204f2126eA#code
Impacts:
Griefing (e.g. no profit motive for an attacker, but damage to the users or the protocol)
Protocol at risk of getting queued transactions executed more than once
Description
Brief/Intro
Please read the following carefully:
This bug makes it possible to unexpectedly execute a timelocked queued transaction more than once, ACCIDENTALLY/MISTAKENLY.
This vulnerability/risk does NOT require attacker access to privileged addresses/multisigs, because there is no attacker to begin with.
Due to this bug, ACCIDENTAL actions can lead to unfavorable/unacceptable impacts/risks on the protocol or users.
Vulnerability Details
Case 1: operations multisig queues transaction, later executes tx and then community multisig executes same tx again.
Case 2: operations multisig queues transaction, community multisig executes tx before timelock delay passes, and then operations multisig executes same tx again after timelock delay.
The bug is related to the lack of proper validation/checks for
queue[txHash] = 0;
.For more details and clarity please see the bugfix section for my fixes.
The bug allows for queued transactions to be executed more than once, which should not be allowed due to the potential risks/impacts involved.
Could potentially cause damage but at least frustration to protocol and/or users if the same transaction was executed twice. By "same transaction" I'm specifically referring to tx with same
operationId
ortxHash
.The invariant being violated by this bug is that
it should never be possible to execute the same queued transaction more than once
.
Type of queued transactions that we dont want to execute twice, i.e. same tx with same operationId executed TWICE:
upgrade a contract
depositing into EigenLayer's stETH strategy contract
redeeming stETH for ETH from EL (via Lido)
pretty much all the transactions that can be queued by operations multisig now and in the future after upgrades
The buggy function:
see my bugfix section after the PoC section:
Impact Details
Potential Impacts in scope:
Griefing: see definition of griefing below, which isnt restricted to only an attacker, could be non-attack griefing too
Contract fails to deliver promised returns, but doesn't lose value: this should include not only financial related but also non-financial related functionality which should not exist. i.e. promised returns should include promised contract functionality.
See more detailed explanations of above two impacts in scope below:
Griefing risk is not necessarily isolated to an attacker, it could happen without an attacker/attack, as explained below:
Griefing can encompass situations where disruption, harm, or inconvenience is caused not only by intentional attacks but also by accidental actions or negligence. While griefing often implies malicious intent, the consequences of actions that result from accidental mistakes or negligence can have similar effects on the protocol and its users.
For example:
Accidental Actions: Suppose a user unintentionally triggers a series of transactions that disrupt the functioning of a decentralized application (DApp) or smart contract, causing financial losses or inconvenience to other users. Even though the action was not malicious, it still results in griefing-like consequences.
Negligence: If developers or administrators of a protocol fail to properly secure or maintain the system, leading to vulnerabilities being exploited or critical errors occurring, the resulting disruptions and losses could be considered a form of griefing, albeit stemming from negligence rather than malicious intent.
In both cases, whether the disruption is caused intentionally or unintentionally, the impact on the protocol and its users can be similar. It may result in economic losses, loss of trust, reputational damage, and the need for mitigation efforts. Therefore, when discussing griefing in the context of Web3 protocols, it's important to consider a broad spectrum of actions and their consequences, including those stemming from accidents or negligence.
The impact "Contract fails to deliver promised returns, but doesn't lose value" can be considered as the contract not behaving as expected, irrespective of whether user funds are involved. In the context of smart contracts, the expected behavior is typically defined by the contract's code and its intended purpose. If the contract fails to fulfill its obligations or execute its functions as intended, it's considered a failure, regardless of whether user funds are directly impacted.
Even if no user funds are involved, such a failure can still have significant repercussions, including loss of trust, reputational damage, legal implications, and economic consequences. Therefore, ensuring that smart contracts behave as expected is crucial for maintaining the integrity and reliability of the entire system, regardless of the specific financial implications.
Therefore both mentioned impacts should be relevant to this bug and bug report, but at least the griefing impact if not both.
IMPACTS:
Even if both the community & operations multisigs were/are fully TRUSTED, this trust factor does not cover accidental actions, and this bug report is about accidental/mistaken actions, and NOT about malicious actions.
If a role/user/multisig is fully trusted, the assumption is that they will never do anything bad out of malicious intention, however, bad things could happen due to accidental/mistaken actions too, and the current implementation has no mitigation against accidental or mistaken transaction executions made possible by this bug.
References
https://github.com/PufferFinance/pufETH/blob/3e76d02c7b54323d347c8277327d3877bab591f5/src/Timelock.sol#L182-L225
Proof of Concept
PoC tests:
PRIMARY PoC tests: main PoCs for this bug report TEST 1: proving the bug exists TEST 2: proving my bugfix works
SECONDARY PoC tests, not included:
(I will investigate the below further and if I think valid, I will do PoCs for them too, and add to this bug report or create a new report, depending on feedback from Immunefi team.): TEST 3: proving that it's possible to execute same upgrade tx twice TEST 4: proving temporary freezing of funds: might not be valid, need to check. TEST 5: proving (temporary) insolvency: might not be valid, need to check.
For this bug report and its PoC I made use of existing test file pufETH/test/Integration/PufferTest.integration.t.sol
, and added my own test function as per below.
Please note: CASE 1 and CASE 2 from the below test function should not be executed at the same time, so execute one of them at a time, and keep the other one commented out. See details in test function below: