Uninitialized local variable may cause unintended behaviour

[code423n4]

Lines of code

https://github.com/GenerationSoftware/pt-v5-vault/blob/b1deb5d494c25f885c34c83f014c8a855c5e2749/src/Vault.sol#L1051

Vulnerability details

Impact

The current implementation of the _claimPrize function in Vault.sol does not include proper validation for the recipient address returned by the beforeClaimPrize hook. As a result, if the hook does not effectively return a valid address, the recipient variable will be left uninitialized, defaulting to the zero address (address(0)). This may cause unintended behaviors. For example, this could lead to the prize being transferred to the zero address if the prize token contract does not handle such a case properly.

Vault.sol#L1051

function _claimPrize(
    address _winner,
    uint8 _tier,
    uint32 _prizeIndex,
    uint96 _fee,
    address _feeRecipient
  ) internal returns (uint256) {
    VaultHooks memory hooks = _hooks[_winner];
    address recipient;

    if (hooks.useBeforeClaimPrize) {
      recipient = hooks.implementation.beforeClaimPrize(_winner, _tier, _prizeIndex);

    } else {
      recipient = _winner;
    }
    //Rest of the code
    //...
}

Proof of Concept

Consider this faulty hook implementation from a winner:

contract FaultyHook {

  constructor() {}

  function beforeClaimPrize(
    address winner,
    uint8 tier,
    uint32 prizeIndex
  ) external returns (address) {
    bool index = false;
    if (index) {
      return address(this);
    }
  }

  //Rest of code
  //...
}

The beforeClaimPrize function does not return anything. As a consequence, the provided test case would pass, leading to a transfer of the prize to the zero address.

diff --git a/test/unit/Vault/Vault.t.sol b/test/unit/Vault/Vault.t.sol
index d0d6d30..7cedec6 100644
--- a/test/unit/Vault/Vault.t.sol
+++ b/test/unit/Vault/Vault.t.sol
@@ -4,6 +4,7 @@ pragma solidity 0.8.17;
 import { UnitBaseSetup, LiquidationPair, PrizePool, TwabController, VaultMock, ERC20, IERC20, IERC4626 } from "test/utils/UnitBaseSetup.t.sol";
 import { IVaultHooks, VaultHooks } from "../../../src/interfaces/IVaultHooks.sol";
 import "src/Vault.sol";
+import { FaultyHook } from "./FaultyHook.sol";

 contract VaultTest is UnitBaseSetup {
   /* ============ Events ============ */
@@ -174,6 +175,33 @@ contract VaultTest is UnitBaseSetup {
     vm.stopPrank();
   }

+  //@audit - This test should pass as the recipient address is set to 0 when claiming prize.
+  function testClaimPrize_faultyHook() public {
+    FaultyHook faultyHook = new FaultyHook();
+    vm.startPrank(alice);
+    VaultHooks memory hooks = VaultHooks({
+      useBeforeClaimPrize: true,
+      useAfterClaimPrize: false,
+      implementation: IVaultHooks(address(faultyHook))
+    });
+    vault.setHooks(hooks);
+    vm.stopPrank();
+
+    vm.startPrank(address(claimer));
+
+    mockPrizePoolClaimPrize(
+      uint8(1),
+      alice,
+      0,
+      address(0) /**recipient address */,
+      1e18,
+      address(claimer)
+    );
+    claimPrize(uint8(1), alice, 0, 1e18, address(claimer));
+
+    vm.stopPrank();
+  }
+
   function testClaimPrize_beforeHook() public {
     vm.startPrank(alice);
     VaultHooks memory hooks = VaultHooks({

Tools Used

Foundry

Recommended Mitigation Steps

To prevent any unexpected behavior, initialize the recipient variable to the winner address:

function _claimPrize(
    address _winner,
    uint8 _tier,
    uint32 _prizeIndex,
    uint96 _fee,
    address _feeRecipient
  ) internal returns (uint256) {
    VaultHooks memory hooks = _hooks[_winner];
    address recipient = _winner;
    //Rest of the code
    //...
}

Alternatively, add a check for the recipient after its assignment by the hook:

 if (hooks.useBeforeClaimPrize) {
      recipient = hooks.implementation.beforeClaimPrize(_winner, _tier, _prizeIndex);
      require(recipient != address(0), "Recipient can't be zero address");
    } else {
      recipient = _winner;
    }

These mitigation steps ensure that the recipient is properly initialized and prevent the prize from being transferred to the zero address.

Assessed type

Other

[c4-judge]

Picodes marked the issue as duplicate of #465

[c4-judge]

Picodes marked the issue as satisfactory