Rental safes can call Guard.checkTransaction directly to execute hooks with arbitrary params

[c4-bot-4]

Lines of code

https://github.com/re-nft/smart-contracts/blob/3ddd32455a849c3c6dc3c3aad7a33a6c9b44c291/src/policies/Guard.sol#L309

Vulnerability details

Impact

When Guard.checkTransaction is executed, onTransaction hooks may be executed according to the data of the transaction. It's possible, however, for rental safes to call checkTransaction directly with arbitrary data to execute hooks with arbitrary parameters.

Note: This finding is unique from the following findings:

• "Possible to call Guard.checkTransaction directly with arbitrary data to fire onTransaction hook"
• "It's possible to get onTransaction hooks to fire without actually executing the underlying function"

As stated in the docs: "Findings are duplicates if they share the same root cause. More specifically, if fixing the Root Cause (in a reasonable manner) would cause the finding to no longer be exploitable, then the findings are duplicates."

These findings all have different root causes and mitigations and therefore are not duplicates.

Proof of Concept

When checkTransaction is called, we check whether the to address has a corresponding hook and has the onTransaction hook enabled, and if so we execute hook.onTransaction with the parameters to be used on the function which we're checking.

// Fetch the hook to interact with for this transaction.
address hook = STORE.contractToHook(to);
bool isActive = STORE.hookOnTransaction(hook);

// If a hook exists and is enabled, forward the control flow to the hook.
if (hook != address(0) && isActive) {
    _forwardToHook(hook, msg.sender, to, value, data);
}

checkTransaction is called by Safe.execTransaction to validate the transaction before execution, blocking certain calls. However, calling checkTransaction itself is not blocked. As a result, it's possible to call checkTransaction directly, with any arbitrary data and it will be passed to the hook as if that data were to be executed by the Safe. This can change state in the hook contracts in unexpected ways.

Tools Used

• Manual review

Recommended Mitigation Steps

Prevent calls from the safe to Guard.checkTransaction, either by blocking all calls to the Guard contract or simply blocking the checkTransaction selector.

Assessed type

Invalid Validation

[c4-pre-sort]

141345 marked the issue as duplicate of #396

[c4-judge]

0xean marked the issue as unsatisfactory: Out of scope

[kadenzipfel]

1. This is falsely marked as a duplicate of https://github.com/code-423n4/2024-01-renft-findings/issues/396. From the docs: "Findings are duplicates if they share the same root cause. More specifically, if fixing the Root Cause (in a reasonable manner) would cause the finding to no longer be exploitable, then the findings are duplicates." Since the reasonable mitigation from #396 leaves this vulnerability present, this is therefore not a duplicate.
2. Similarly to https://github.com/code-423n4/2024-01-renft-findings/issues/170, the impact of this issue is that the hooks can be unexpectedly executed with the rental safe as msg.sender, which is more significant considering that the hook logic is applied to rental safe's rather than arbitrary accounts like in #396. As such, the severity of this finding should be consistent with #170.

[0xean]

170 may end up as QA as well. Waiting for sponsor comment before marking it down.

This can change state in the hook contracts in unexpected ways.

Is a very unclear impact statement that probably doesn't quality for M regardless