All assets controlled by safe could be inaccessible due to Deny of Service

[c4-submissions]

Lines of code

https://github.com/code-423n4/2023-10-brahma/blob/main/contracts/src/core/SafeModeratorOverridable.sol#L16 https://github.com/code-423n4/2023-10-brahma/blob/main/contracts/src/core/SafeModerator.sol#L16

Vulnerability details

Impact

All assets controlled by safe could be inaccessible due to Denial of Service caused by combination of safe upgrade and the flaw of SafeModeratorOverridable and SafeModerator

Proof of Concept

• SafeModeratorOverridable is the guard of console account
• SafeModerator is the guard of sub account

Once the guard of safe account is set, each transaction will be checked by the guard:

171:        address guard = getGuard();
172:        {
173:            if (guard != address(0)) {
174:                Guard(guard).checkTransaction(
175:                    // Transaction info
176:                    to,
177:                    value,
178:                    data,
179:                    operation,
180:                    safeTxGas,
181:                    // Payment info
182:                    baseGas,
183:                    gasPrice,
184:                    gasToken,
185:                    refundReceiver,
186:                    // Signature info
187:                    signatures,
188:                    msg.sender
189:                );
190:            }
191:        }

if somehow the guard is unable to work properly, the safe transaction execution could be blocked and there is no way to access all assets locked in the safe.

As we all know that Gnosis Safe uses a Proxy/Implementation pattern, which means the implementation of safe smart contracts can be upgraded. But the process of transaction execution could be changed and there is no guarantee that the new version is compatible with previous guards.

For example, if the name of check function changes, any safe using SafeModeratorOverridable or SafeModerator as guard will be locked because the new function identifier used in Safe.sol doesn't match any of the available functions provided by SafeModeratorOverridable or SafeModerator.

Tools Used

Manual review

Recommended Mitigation Steps

It is recommended to add fallback() function in SafeModeratorOverridable and SafeModerator, which has been adopted by Zodiac and Yearn. All examples provide by Gnosis Safe did this as well:

fallback() external {}

Assessed type

DoS

[c4-pre-sort]

raymondfam marked the issue as low quality report

[c4-pre-sort]

raymondfam marked the issue as primary issue

[alex-ppg]

The severity of this exhibit is greatly over-inflated, however, the discrepancy identified by the Warden is correct; a Gnosis Safe may be upgraded to a version that utilizes its guard differently, causing the wallet to be blocked.

The introduction of an empty fallback method is present since v1.3.0 that the Brahma system should be compatible with and it is valid to advise the Sponsor to introduce it in the SafeModerator implementations.

[c4-judge]

alex-ppg marked the issue as unsatisfactory: Overinflated severity