hash - `maxRedeem` doesn't comply with ERC-4626

[sherlock-admin4]

hash

medium

maxRedeem doesn't comply with ERC-4626

Summary

maxRedeem function reverts due to division by 0 and hence doesn't comply with ERC4626

Vulnerability Detail

The contract's maxRedeem function doesn't comply with ERC-4626 which is a mentioned requirement. According to the specification, MUST NOT revert.
The maxRedeem function will revert in case the totalAsset amount is 0 due to a divison by 0

link

uint256 _maxScaledRedeem = _convertToShares(_maxWithdraw, _totalAssets, totalDebt(), Math.Rounding.Up);

link

    function _convertToShares(
        uint256 _assets,
        uint256 _totalAssets,
        uint256 _totalDebt,
        Math.Rounding _rounding
    ) internal pure returns (uint256) {
        if (_totalAssets >= _totalDebt) {
            return _assets;
        } else {
            return _assets.mulDiv(_totalDebt, _totalAssets, _rounding);

POC

diff --git a/pt-v5-vault/test/unit/PrizeVault/PrizeVault.t.sol b/pt-v5-vault/test/unit/PrizeVault/PrizeVault.t.sol
index 7006f44..9b92c35 100644
--- a/pt-v5-vault/test/unit/PrizeVault/PrizeVault.t.sol
+++ b/pt-v5-vault/test/unit/PrizeVault/PrizeVault.t.sol
@@ -4,6 +4,8 @@ pragma solidity ^0.8.24;
 import { UnitBaseSetup, PrizePool, TwabController, ERC20, IERC20, IERC4626, YieldVault } from "./UnitBaseSetup.t.sol";
 import { IPrizeHooks, PrizeHooks } from "../../../src/interfaces/IPrizeHooks.sol";
 import { ERC20BrokenDecimalMock } from "../../contracts/mock/ERC20BrokenDecimalMock.sol";
+import "forge-std/Test.sol";
+

 import "../../../src/PrizeVault.sol";

@@ -45,6 +47,34 @@ contract PrizeVaultTest is UnitBaseSetup {
         assertEq(testVault.owner(), address(this));
     }

+    function testHash_MaxRedeemRevertDueToDivByZero() public {
+        PrizeVault testVault=  new PrizeVault(
+            "PoolTogether aEthDAI Prize Token (PTaEthDAI)",
+            "PTaEthDAI",
+            yieldVault,
+            PrizePool(address(prizePool)),
+            claimer,
+            address(this),
+            YIELD_FEE_PERCENTAGE,
+            0,
+            address(this)
+        );
+
+        uint amount_ = 1e18;
+        underlyingAsset.mint(address(this),amount_);
+
+        underlyingAsset.approve(address(testVault),amount_);
+
+        testVault.deposit(amount_,address(this));
+
+        // lost the entire deposit amount
+        underlyingAsset.burn(address(yieldVault), 1e18);
+
+        vm.expectRevert();
+        testVault.maxRedeem(address(this));
+
+    }
+
     function testConstructorYieldVaultZero() external {
         vm.expectRevert(abi.encodeWithSelector(PrizeVault.YieldVaultZeroAddress.selector));

Impact

Failure to comply with the specification which is a mentioned necessity

Code Snippet

Tool used

Manual Review

Recommendation

Handle the _totalAssets == 0 condition

[sherlock-admin3]

1 comment(s) were left on this issue during the judging contest.

infect3d commented:

if totalAssets is 0 the first if statement will be executed thus no revert

[nevillehuang]

Valid medium since it was mentioned as:

Is the codebase expected to comply with any EIPs? Can there be/are there any deviations from the specification? PrizeVaults are expected to strictly comply with the ERC4626 standard.

Sherlock rules states

The protocol team can use the README (and only the README) to define language that indicates the codebase's restrictions and/or expected functionality. Issues that break these statements, irrespective of whether the impact is low/unknown, will be assigned Medium severity

[InfectedIsm]

@nevillehuang @WangSecurity I think you didn't take my judging comment into consideration, sorry for acting so late.

If totalAssets == 0, this means totalDebt == 0 too as:

• at start, the prize vault gets assets deposited as the yield buffer so totalAssets > 0
• debt is minted 1:1 with deposited assets, so TotalAssets > TotalDebt during the existence of the vault
• when yield is accrued, TotalAssets is increased, while debt is as most increased up to totalAssets for yield to be liquidated
• when the Yield Vault take a loss for any reason, then assets<debt, a user withdrawing at this moment shares (debt) burned > asset withdrawn, until debt = assets
• This means, if totalAssets == 0, then totalDebt == 0 too

Hence, this is the case if (_totalAssets >= _totalDebt) that is executed, and not the else case, so no division by 0 here.

The only case where this could happen is a yield vault being hacked and fully withdrawn from its asset, and the yield buffer of the prize vault fully emptied If this cannot happen in normal circonstances (yield vault compliant, so no way to get hacked) then we should consider this case cannot happen, so the function cannot revert.

@10xhash FYI

[sherlock-admin2]

The protocol team fixed this issue in the following PRs/commits: https://github.com/GenerationSoftware/pt-v5-vault/pull/112

[10xhash]

Fixed Now maxRedeem returns 0 instead of reverting

[sherlock-admin2]

The Lead Senior Watson signed off on the fix.