search

sherlock-audit / 2024-01-telcoin-judging

6 stars 5 forks source link

fnanni - Vetoed transactions are not uniquely identified in SafeGuard's checkTransaction() #198

Closed sherlock-admin closed 6 months ago

sherlock-admin commented 6 months ago

fnanni

medium

Vetoed transactions are not uniquely identified in SafeGuard's checkTransaction()

Summary

Transaction data (to, value, data, operation) does not uniquely identify transactions. As a consequence transactions vetoed at a given time will get vetoed forever, even if coming from different snapshot proposals.

Vulnerability Detail

Let's say a transaction in which Alice gets sent 1 ether is vetoed and any nonce is used for that. If the same transaction is approved by the DAO one year later, the transaction will still be blocked until the Guard is changed to a bug-free one. This happens because all nonces are checked for a given transaction.

Impact

Transactions can accidentally get vetoed forever. Usually there should be workarounds to unblock this state: change the guard, use intermediary contracts to transfer tokens or change transactions data slightly so that the transaction hash is not repeated, etc. However, proposal executions can expire in the meanwhile causing unnecessary friction in the DAO and even losses for untaken opportunities (for example not withdrawing/staking tokens from/in a protocol in time).

Code Snippet

https://github.com/sherlock-audit/2024-01-telcoin/blob/0954297f4fefac82d45a79c73f3a4b8eb25f10e9/telcoin-audit/contracts/zodiac/core/SafeGuard.sol#L28-L75

Tool used

Manual Review

Recommendation

Consider either:

  1. using RealityModule's invalidation feature, which vetoes whole proposals (See here and here).
  2. Veto transactions based on Reality's question hashes and transaction indexes (currently referred as nonces).

Also try to reduce iterations inside the Guard's checkTransaction() function (or remove them completely if possible). One way of reducing them, is by storing nonces in arrays per transaction.

Duplicate of #209

sherlock-admin2 commented 6 months ago

1 comment(s) were left on this issue during the judging contest.

takarez commented:

invalid because {veto is permanent i believe; so intended behavior}