search

sherlock-audit / 2024-01-telcoin-judging

6 stars 5 forks source link

fnanni - Guard is wrongly implemented and will freeze the Safe forever once the first transaction hash is vetoed #110

Closed sherlock-admin closed 8 months ago

sherlock-admin commented 9 months ago

fnanni

high

Guard is wrongly implemented and will freeze the Safe forever once the first transaction hash is vetoed

Summary

For Safe versions <= 1.4.1: modules get to execute transactions on the corresponding Safe by-passing guards. This means that transactions coming from the Zodiac module cannot be vetoed.

For Safe versions that include module-guarded transactions: the function that should be used is checkModuleTransaction(), not checkTransaction(). Transactions coming from the Zodiac module can be vetoed, but the current implementation fails to do so, and, worse, actually all of them get vetoed because the function is not implemented and will revert. This would be the worst scenario, because the Safe and funds in it could get frozen forever.

Vulnerability Detail

Safe versions <= 1.4.1

Safe Modules execute transactions on safes through the execTransactionFromModule() function. In Safe versions <= 1.4.1, the guard doesn't get called here. This means the transactions cannot get vetoed. Additionally, for direct transactions to the Safe (not from modules), checkTransaction() would revert trying to call getTransactionHash() on the caller address if there is at least one transaction vetoed.

Note that, even if execTransactionFromModule() called the guard, _msgSender() in L63 is NOT the zodiac module but the Gnosis safe. The call path is Zodiac Module --> Gnosis Safe --> Guard. The Gnosis safe contract doesn't have a getTransactionHash function, which means the checkTransaction(...) would revert anyway. This happens if the function enters the for loop, which will always do after the owner of the contract calls vetoTransaction(...) for the first time.

Safe versions with module-guarded transactions

In the current main implementation of ModuleManager.sol, the guard is called. Note that checkModuleTransaction() needs to be implemented in the guard and that the zodiac module address is passed in the last parameter. Be careful of not using msg.sender in the Guard implementation. Otherwise, getTransactionHash will be called on the Safe instead of on the zodiac module.

Impact

Depends on that Gnosis Safe release. Currently the guard is unable to veto transactions and could potentially freeze the Safe.

Code Snippet

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

https://github.com/sherlock-audit/2024-01-telcoin/blob/main/telcoin-audit/contracts/zodiac/core/BaseGuard.sol

To the team: Please be careful when writing tests. Don't just assume that the calls revert the way you think they revert. Tests here pass, but for the wrong reasons. Also make sure to add mock contracts for the whole execution path.

Tool used

Manual Review

Recommendation

sherlock-admin2 commented 9 months ago

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

takarez commented:

invalid because {no POC}

nevillehuang commented 8 months ago

request poc

sherlock-admin2 commented 8 months ago

PoC requested from @fnanni-0

Requests remaining: 10

fnanni-0 commented 8 months ago

EDIT: ignore this comment, the issue is invalid.

@nevillehuang I think it will suffice to rephrase the description of the vulnerability. The implementation of the Guard has 2 basic problems.

1. Who calls the guard contract?

The guard contract's checkTransaction() function is always called by the Safe contract as illustrated in the Safe docs: Guard flowchart

It's important to note that Safe modules are NOT part of the Safe itself. They are separate contracts. Modules interact with the Safe by calling execTransactionFromModule() as illustrated in the Safe docs: Module flowchart

This means that the contract execution path is Module --> Safe --> Guard. You can verify this in the Safe contract (see the external call to the guard here) and in the RealityModule contract (you won't find any calls to a guard contract, only to the safe).

So, why is the guard implementation wrong? It assumes that the checkTransaction() caller is the RealityModule contract instead of the Safe contract. In the following line, SafeGuard will try to call getTransactionHash() on the Safe, which will be handled by the Safe fallbackHandler and therefore will likely revert blocking every transaction.

bytes32 transactionHash = IReality(_msgSender()).getTransactionHash(

Even the interface used is incorrect, as it confuses Reality with RealityModule, but the interface being incorrect doesn't do any additional harm.

2. Can Guards check calls coming from Modules?

Currently no. Up until the latest release of the Safe contracts (v1.4.1-build.0 at the time of writing), transactions coming from modules are never reviewed by the Guard contract. You can easily check this in the Safe's ModuleManager.

Good news to Telcoin is that the Gnosis team seems to be working on a new Safe version that will include Guard checks for modules. Check the ModuleManager code in the main branch of the safe contracts here. Note however that the function selector is checkModuleTransaction() which is not implemented in Telcoin's SafeGuard.sol

Impact

Currently the Guard is useless, as it is unable to veto transactions coming from modules. If the Telcoin team waits until Gnosis releases a Safe version with guarded modules, then the guard could work, but they would have to rewrite the Safe Guard to match the new requirements (otherwise the guard will block every transaction and the Safe will freeze).

amshirif commented 8 months ago

The issue described is using the wrong Gnosis guard. The transaction flow of Module --> Safe --> Guard is mistaken. In the Zodiac guard set up the ordering is Module --> Guard --> Safe. Here is a sample transaction 0x064b7422a5e3de9c9f34bf22f858dc270aae9a5d53b20c2296bfa5415dde0570. These results have been verified to be false. A transaction was vetoed on the above guard and the transaction proceeded as expected. The explanation also said that the RealityModule was incorrect. Here is the module used RealityModuleETH

fnanni-0 commented 8 months ago

@amshirif you are right. I just realized that that the RealityModule uses an old zodiac version: "@gnosis.pm/zodiac": "1.0.1". I was looking at a newer one. Sorry for the misunderstanding.