Merge pull request #118 from GenerationSoftware/fix-review

Sherlock fix review

.gitmodules

@@ -37,3 +37,6 @@
 [submodule "lib/yield-daddy"]
 	path = lib/yield-daddy
 	url = https://github.com/timeless-fi/yield-daddy
+[submodule "lib/ExcessivelySafeCall"]
+	path = lib/ExcessivelySafeCall
+	url = https://github.com/nomad-xyz/ExcessivelySafeCall

remappings.txt

@@ -6,6 +6,7 @@ brokentoken/=lib/brokentoken/src/
 openzeppelin/=lib/openzeppelin-contracts/contracts/
 owner-manager-contracts/=lib/owner-manager-contracts/contracts/
 yield-daddy/=lib/yield-daddy/src/
+excessively-safe-call/=lib/ExcessivelySafeCall/src/
 
 pt-v5-liquidator-interfaces/=lib/pt-v5-liquidator-interfaces/src/interfaces/
 pt-v5-twab-controller=lib/pt-v5-twab-controller/src/

src/PrizeVault.sol

@@ -12,7 +12,7 @@ import { TwabERC20 } from "./TwabERC20.sol";
 
 import { ILiquidationSource } from "pt-v5-liquidator-interfaces/ILiquidationSource.sol";
 import { PrizePool } from "pt-v5-prize-pool/PrizePool.sol";
-import { TwabController, SPONSORSHIP_ADDRESS } from "pt-v5-twab-controller/TwabController.sol";
+import { TwabController } from "pt-v5-twab-controller/TwabController.sol";
 
 /// @dev The TWAB supply limit is the max number of shares that can be minted in the TWAB controller.
 uint256 constant TWAB_SUPPLY_LIMIT = type(uint96).max;
@@ -152,12 +152,6 @@ contract PrizeVault is TwabERC20, Claimable, IERC4626, ILiquidationSource, Ownab
     /// @param yieldFeePercentage New yield fee percentage
     event YieldFeePercentageSet(uint256 yieldFeePercentage);
 
-    /// @notice Emitted when a user sponsors the Vault.
-    /// @param caller Address that called the function
-    /// @param assets Amount of assets deposited into the Vault
-    /// @param shares Amount of shares minted to the caller address
-    event Sponsor(address indexed caller, uint256 assets, uint256 shares);
-
     /// @notice Emitted when yield is transferred out by the liquidation pair address.
     /// @param liquidationPair The liquidation pair address that initiated the transfer
     /// @param tokenOut The token that was transferred out
@@ -377,13 +371,16 @@ contract PrizeVault is TwabERC20, Claimable, IERC4626, ILiquidationSource, Ownab
     /// @dev Considers the TWAB mint limit
     /// @dev Returns zero if any deposit would result in a loss of assets
     /// @dev Returns zero if total assets cannot be determined
+    /// @dev Returns zero if the yield buffer is less than half full. This is a safety precaution to ensure
+    /// a deposit of the asset amount returned by this function cannot reasonably trigger a `LossyDeposit`
+    /// revert in the `deposit` or `mint` functions if the yield buffer has been configured properly.
     /// @dev Any latent balance of assets in the prize vault will be swept in with the deposit as a part of
     /// the "dust collection strategy". This means that the max deposit must account for the latent balance
     /// by subtracting it from the max deposit available otherwise.
     function maxDeposit(address /* receiver */) public view returns (uint256) {
         uint256 _totalDebt = totalDebt();
         (bool _success, uint256 _totalAssets) = _tryGetTotalPreciseAssets();
-        if (!_success || _totalAssets < _totalDebt) return 0;
+        if (!_success || _totalAssets < _totalDebt + yieldBuffer / 2) return 0;
 
         uint256 _latentBalance = _asset.balanceOf(address(this));
         uint256 _maxYieldVaultDeposit = yieldVault.maxDeposit(address(this));
@@ -551,25 +548,6 @@ contract PrizeVault is TwabERC20, Claimable, IERC4626, ILiquidationSource, Ownab
         return _shares;
     }
 
-    /// @notice Deposit assets into the Vault and delegate to the sponsorship address.
-    /// @dev Emits a `Sponsor` event
-    /// @param _assets Amount of assets to deposit
-    /// @return Amount of shares minted to caller.
-    function sponsor(uint256 _assets) external returns (uint256) {
-        address _owner = msg.sender;
-
-        uint256 _shares = previewDeposit(_assets);
-        _depositAndMint(_owner, _owner, _assets, _shares);
-
-        if (twabController.delegateOf(address(this), _owner) != SPONSORSHIP_ADDRESS) {
-            twabController.sponsor(_owner);
-        }
-
-        emit Sponsor(_owner, _assets, _shares);
-
-        return _shares;
-    }
-
     ////////////////////////////////////////////////////////////////////////////////
     // Additional Withdrawal Flows
     ////////////////////////////////////////////////////////////////////////////////
@@ -686,13 +664,22 @@ contract PrizeVault is TwabERC20, Claimable, IERC4626, ILiquidationSource, Ownab
     /// @dev Supports the liquidation of either assets or prize vault shares.
     function liquidatableBalanceOf(address _tokenOut) external view returns (uint256) {
         uint256 _totalDebt = totalDebt();
+        uint256 _yieldFeePercentage = yieldFeePercentage;
         uint256 _maxAmountOut;
         if (_tokenOut == address(this)) {
             // Liquidation of vault shares is capped to the mint limit.
             _maxAmountOut = _mintLimit(_totalDebt);
         } else if (_tokenOut == address(_asset)) {
             // Liquidation of yield assets is capped at the max yield vault withdraw plus any latent balance.
             _maxAmountOut = _maxYieldVaultWithdraw() + _asset.balanceOf(address(this));
+
+            // If a yield fee will be minted, then the liquidation will also be capped based on the remaining mint limit.
+            if (_yieldFeePercentage != 0) {
+                uint256 _maxAmountBasedOnFeeMintLimit = _mintLimit(_totalDebt).mulDiv(FEE_PRECISION, _yieldFeePercentage);
+                if (_maxAmountBasedOnFeeMintLimit < _maxAmountOut) {
+                    _maxAmountOut = _maxAmountBasedOnFeeMintLimit;
+                }
+            }
         } else {
             return 0;
         }
@@ -705,7 +692,7 @@ contract PrizeVault is TwabERC20, Claimable, IERC4626, ILiquidationSource, Ownab
         // The final balance is computed by taking the liquid yield and multiplying it by
         // (1 - yieldFeePercentage), rounding down, to ensure that enough yield is left for
         // the yield fee.
-        return _liquidYield.mulDiv(FEE_PRECISION - yieldFeePercentage, FEE_PRECISION);
+        return _liquidYield.mulDiv(FEE_PRECISION - _yieldFeePercentage, FEE_PRECISION);
     }
 
     /// @inheritdoc ILiquidationSource
@@ -856,6 +843,7 @@ contract PrizeVault is TwabERC20, Claimable, IERC4626, ILiquidationSource, Ownab
     }
 
     /// @notice Converts assets to shares with the given vault state and rounding direction.
+    /// @dev Returns zero if the vault is in a lossy state AND there are no more assets.
     /// @param _assets The assets to convert
     /// @param _totalAssets The total assets that the vault controls
     /// @param _totalDebt The total debt the vault owes
@@ -873,7 +861,7 @@ contract PrizeVault is TwabERC20, Claimable, IERC4626, ILiquidationSource, Ownab
             // If the vault controls less assets than what has been deposited a share will be worth a
             // proportional amount of the total assets. This can happen due to fees, slippage, or loss
             // of funds in the underlying yield vault.
-            return _assets.mulDiv(_totalDebt, _totalAssets, _rounding);
+            return _totalAssets == 0 ? 0 : _assets.mulDiv(_totalDebt, _totalAssets, _rounding);
         }
     }
 

src/abstract/Claimable.sol

@@ -3,14 +3,17 @@ pragma solidity ^0.8.24;
 
 import { IClaimable } from "pt-v5-claimable-interface/interfaces/IClaimable.sol";
 import { PrizePool } from "pt-v5-prize-pool/PrizePool.sol";
+import { ExcessivelySafeCall  } from "excessively-safe-call/ExcessivelySafeCall.sol";
 
 import { HookManager } from "./HookManager.sol";
+import { IPrizeHooks } from "../interfaces/IPrizeHooks.sol";
 
 /// @title  PoolTogether V5 Claimable Vault Extension
 /// @author G9 Software Inc.
 /// @notice Provides an interface for Claimer contracts to interact with a vault in PoolTogether
 /// V5 while allowing each account to set and manage prize hooks that are called when they win.
 abstract contract Claimable is HookManager, IClaimable {
+    using ExcessivelySafeCall for address;
 
     ////////////////////////////////////////////////////////////////////////////////
     // Public Constants and Variables
@@ -20,6 +23,13 @@ abstract contract Claimable is HookManager, IClaimable {
     /// @dev This should be enough gas to mint an NFT if needed.
     uint24 public constant HOOK_GAS = 150_000;
 
+    /// @notice The number of bytes to limit hook return / revert data.
+    /// @dev If this limit is exceeded for `beforeClaimPrize` return data, the claim will revert.
+    /// @dev Revert data for both hooks will also be limited to this size.
+    /// @dev 128 bytes is enough for `beforeClaimPrize` to return the `_prizeRecipient` address as well
+    /// as 32 bytes of additional `_hookData` byte string data (32 for offset, 32 for length, 32 for data).
+    uint16 public constant HOOK_RETURN_DATA_LIMIT = 128;
+
     /// @notice Address of the PrizePool that computes prizes.
     PrizePool public immutable prizePool;
 
@@ -44,6 +54,11 @@ abstract contract Claimable is HookManager, IClaimable {
     /// @param claimer The claimer address
     error CallerNotClaimer(address caller, address claimer);
 
+    /// @notice Thrown if relevant hook return data is greater than the `HOOK_RETURN_DATA_LIMIT`.
+    /// @param returnDataSize The actual size of the return data
+    /// @param hookDataLimit The return data size limit for hooks
+    error ReturnDataOverLimit(uint256 returnDataSize, uint256 hookDataLimit);
+
     ////////////////////////////////////////////////////////////////////////////////
     // Modifiers
     ////////////////////////////////////////////////////////////////////////////////
@@ -73,6 +88,7 @@ abstract contract Claimable is HookManager, IClaimable {
 
     /// @inheritdoc IClaimable
     /// @dev Also calls the before and after claim hooks if set by the winner.
+    /// @dev Reverts if the return data size of the `beforeClaimPrize` hook exceeds `HOOK_RETURN_DATA_LIMIT`.
     function claimPrize(
         address _winner,
         uint8 _tier,
@@ -84,13 +100,23 @@ abstract contract Claimable is HookManager, IClaimable {
         bytes memory _hookData;
 
         if (_hooks[_winner].useBeforeClaimPrize) {
-            (_prizeRecipient, _hookData) = _hooks[_winner].implementation.beforeClaimPrize{ gas: HOOK_GAS }(
-                _winner,
-                _tier,
-                _prizeIndex,
-                _reward,
-                _rewardRecipient
+            (bytes memory _returnData, uint256 _actualReturnDataSize) = _safeHookCall(
+                _hooks[_winner].implementation,
+                abi.encodeWithSelector(
+                    IPrizeHooks.beforeClaimPrize.selector,
+                    _winner,
+                    _tier,
+                    _prizeIndex,
+                    _reward,
+                    _rewardRecipient
+                )
             );
+            // If the actual return data is greater than the `HOOK_RETURN_DATA_LIMIT` then we must revert since the
+            // integrity of the data is not guaranteed.
+            if (_actualReturnDataSize > HOOK_RETURN_DATA_LIMIT) {
+                revert ReturnDataOverLimit(_actualReturnDataSize, HOOK_RETURN_DATA_LIMIT);
+            }
+            (_prizeRecipient, _hookData) = abi.decode(_returnData, (address, bytes));
         } else {
             _prizeRecipient = _winner;
         }
@@ -107,13 +133,17 @@ abstract contract Claimable is HookManager, IClaimable {
         );
 
         if (_hooks[_winner].useAfterClaimPrize) {
-            _hooks[_winner].implementation.afterClaimPrize{ gas: HOOK_GAS }(
-                _winner,
-                _tier,
-                _prizeIndex,
-                _prizeTotal - _reward,
-                _prizeRecipient,
-                _hookData
+            _safeHookCall(
+                _hooks[_winner].implementation,
+                abi.encodeWithSelector(
+                    IPrizeHooks.afterClaimPrize.selector,
+                    _winner,
+                    _tier,
+                    _prizeIndex,
+                    _prizeTotal - _reward,
+                    _prizeRecipient,
+                    _hookData
+                )
             );
         }
 
@@ -132,5 +162,36 @@ abstract contract Claimable is HookManager, IClaimable {
         claimer = _claimer;
         emit ClaimerSet(_claimer);
     }
+
+    /// @notice Uses ExcessivelySafeCall to limit the return data size to a safe limit.
+    /// @dev This is used for both hook calls to prevent gas bombs that can be triggered using a large
+    /// amount of return data or a large revert string.
+    /// @dev In the case of an unsuccessful call, the revert reason will be bubbled up if it is within
+    /// the safe data limit. Otherwise, a `ReturnDataOverLimit` reason will be thrown.
+    /// @return _returnData The safe, size limited return data
+    /// @return _actualReturnDataSize The actual return data size of the original result
+    function _safeHookCall(IPrizeHooks _implementation, bytes memory _calldata) internal returns (bytes memory _returnData, uint256 _actualReturnDataSize) {
+        bool _success;
+        (_success, _returnData) = address(_implementation).excessivelySafeCall(
+            HOOK_GAS,
+            0, // value
+            HOOK_RETURN_DATA_LIMIT,
+            _calldata
+        );
+        assembly {
+            _actualReturnDataSize := returndatasize()
+        }
+
+        if (!_success) {
+            // If we can't access the full revert data, we use a generic revert
+            if (_actualReturnDataSize > HOOK_RETURN_DATA_LIMIT) {
+                revert ReturnDataOverLimit(_actualReturnDataSize, HOOK_RETURN_DATA_LIMIT);
+            }
+            // Otherwise, we use a low level revert to bubble up the revert reason
+            assembly {
+                revert(add(32, _returnData), mload(_returnData))
+            }
+        }
+    }
 
 }

test/integration/BaseIntegration.t.sol

@@ -28,7 +28,6 @@ abstract contract BaseIntegration is Test, Permit {
     event MockContribute(address prizeVault, uint256 amount);
     event ClaimYieldFeeShares(address indexed recipient, uint256 shares);
     event TransferYieldOut(address indexed liquidationPair, address indexed tokenOut, address indexed recipient, uint256 amountOut, uint256 yieldFee);
-    event Sponsor(address indexed caller, uint256 assets, uint256 shares);
 
     /* ============ variables ============ */
 

test/invariant/PrizeVault/LossyPrizeVaultInvariant.t.sol

@@ -18,7 +18,8 @@ contract LossyPrizeVaultInvariant is Test {
         uint256 totalPreciseAssets = lossyVaultHarness.vault().totalPreciseAssets();
         uint256 totalDebt = lossyVaultHarness.vault().totalDebt();
         uint256 totalSupply = lossyVaultHarness.vault().totalSupply();
-        if (totalDebt > totalPreciseAssets || type(uint96).max - totalSupply == 0) {
+        uint256 yieldBuffer = lossyVaultHarness.vault().yieldBuffer();
+        if (totalDebt + yieldBuffer / 2 > totalPreciseAssets || type(uint96).max - totalSupply == 0) {
             assertEq(lossyVaultHarness.vault().maxDeposit(address(this)), 0);
         } else {
             assertGt(lossyVaultHarness.vault().maxDeposit(address(this)), 0);

test/invariant/PrizeVault/PrizeVaultFuzzHarness.sol

@@ -50,8 +50,6 @@ contract PrizeVaultFuzzHarness is Permit, StdCheats, StdUtils {
     address public joe;
     uint256 public joePrivateKey;
 
-    address public constant SPONSORSHIP_ADDRESS = address(1);
-
     PrizeVault public vault;
     string public vaultName = "PoolTogether Test Vault";
     string public vaultSymbol = "pTest";
@@ -297,19 +295,6 @@ contract PrizeVaultFuzzHarness is Permit, StdCheats, StdUtils {
         vm.stopPrank();
     }
 
-    /* ============ sponsor ============ */
-
-    function sponsor(uint256 callerSeed, uint256 assets) public useCurrentTime useActor(callerSeed) {
-        assets = _bound(assets, 0, vault.maxDeposit(currentActor));
-        assets = _bound(assets, 0, _maxDealAssets());
-        _dealAssets(currentActor, assets);
-        IERC20(vault.asset()).approve(address(vault), assets);
-
-        vm.expectEmit();
-        emit Deposit(currentActor, currentActor, assets, vault.previewDeposit(assets));
-        vault.sponsor(assets);
-    }
-
     /* ============ claimYieldFeeShares ============ */
 
     function claimYieldFeeShares(uint256 callerSeed, uint256 shares) public useCurrentTime useActor(callerSeed) {