Input validation concerns in the setHooks function in the HookManager contract

[c4-bot-9]

Lines of code

https://github.com/code-423n4/2024-03-pooltogether/blob/main/pt-v5-vault/src/abstract/HookManager.sol#L29

Vulnerability details

Impact

The setHooks function directly sets user-specified hooks without any validation checks. It's essential to validate that the hooks address provided is a legitimate contract that implements the VaultHooks interface correctly.

Proof of Concept

The provided code snippet for setHooks directly assigns user-provided hooks without any form of validation.

 /// @notice Sets the hooks for a winner.
/// @dev Emits a `SetHooks` event
/// @param hooks The hooks to set
function setHooks(VaultHooks calldata hooks) external {
    _hooks[msg.sender] = hooks;
    emit SetHooks(msg.sender, hooks);
}

Tools Used

VS code

Recommended Mitigation Steps

To ensure the hooks provided are indeed valid contracts. This can be achieved by using OpenZeppelin's Address utility for contract checks.

1. Import the Address library:

import "@openzeppelin/contracts/utils/Address.sol";

2. Validate the VaultHooks address within the setHooks function:

function setHooks(VaultHooks calldata hooks) external {
require(Address.isContract(address(hooks)), "Hooks must be a valid contract");
_hooks[msg.sender] = hooks;
emit SetHooks(msg.sender, hooks);
}

It is crucial for DeFi protocols, especially those involving value transfer and external contract interactions like PoolTogether, to rigorously validate user inputs and external contract references to safeguard against vulnerabilities and ensure system robustness.

Assessed type

Access Control

[c4-pre-sort]

raymondfam marked the issue as insufficient quality report

[c4-pre-sort]

raymondfam marked the issue as primary issue

[raymondfam]

User's input error is not a protocol threat. Additionally, this will be circumvented via try/catch by claimer.sol:

https://github.com/GenerationSoftware/pt-v5-claimer/blob/main/src/Claimer.sol#L192

[hansfriese]

QA is more appropriate.

[c4-judge]

hansfriese changed the severity to QA (Quality Assurance)

[c4-judge]

hansfriese marked the issue as grade-c

[c4-judge]

This previously downgraded issue has been upgraded by hansfriese

[c4-judge]

hansfriese removed the grade

[c4-judge]

hansfriese changed the severity to QA (Quality Assurance)

[c4-judge]

hansfriese marked the issue as grade-c