Signers can add new modules to a safe by abusing reentrancy (No Reentrancy checks in the system)

[c4-submissions]

Lines of code

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

Vulnerability details

Impact

Signers who are above the threshold will can give themselves unlimited access over the safe.

Proof of Concept

The checkAfterExecution() function has no checks to ensure that new modules cannot be added by signers. This is a crucial check, because adding a new module could give them unlimited power to make any changes (with no guards in place) in the future.

However, by abusing reentrancy, the parameters used can be changed so that this crucial restriction is violated.

The checkAfterExecution() is intended to uphold important invariants after each signer transaction is completed. This is intended to restrict certain dangerous signer behaviors, the most important of which is adding new modules.

    /**
     * @notice Inherited from IGuard, function is called after executing a Safe transaction during execTransaction
     * @param txHash tx hash, computed from transaction
     * @param success boolean indicating success
     */
    function checkAfterExecution(bytes32 txHash, bool success) external view override {
        TransactionValidator(AddressProviderService._getAuthorizedAddress(_TRANSACTION_VALIDATOR_HASH))
            .validatePostTransaction(txHash, success, msg.sender);
    }

Why Restricting Modules is Important?

Modules are the most important thing to check. This is because modules have unlimited power not only to execute transactions but to skip checks in the future.

Creating an arbitrary new module is so bad that it is equivalent to the other two issues together: getting complete control over the safe (threshold = 1) and removing the guard (because they aren't checked in module transactions).

However, this important restriction can be violated by abusing reentrancy into this function.

Reentrancy:

The protocol is NOT attempting to guard against reentrancy with the guardEntries variable.

In order for this system to work correctly, the checkTransaction() function should simply set _guardEntries = 1.

This would result in an underflow with the second decrement. But, as it is currently designed, there is no reentrancy protection.

Refer this link for more details -
sherlock-audit/2023-02-hats-judging#41

Tools Used

Manual

Recommended Mitigation Steps

checkTransaction() function should set _guardEntries = 1.

This would result in an underflow with the second decrement. But, as it is currently designed, there is no reentrancy protection.

Assessed type

Reentrancy

[c4-pre-sort]

raymondfam marked the issue as low quality report

[c4-pre-sort]

raymondfam marked the issue as duplicate of #4

[c4-judge]

alex-ppg marked the issue as not a duplicate

[c4-judge]

alex-ppg marked the issue as duplicate of #353

[c4-judge]

alex-ppg marked the issue as unsatisfactory:
Invalid

[c4-judge]

alex-ppg marked the issue as unsatisfactory:
Invalid

[c4-judge]

alex-ppg marked the issue as unsatisfactory:
Invalid