hash - maxDeposit doesn't comply with ERC-4626

[sherlock-admin2]

hash

medium

maxDeposit doesn't comply with ERC-4626

Summary

maxDeposit doesn't comply with ERC-4626 since depositing the returned amount can cause reverts

Vulnerability Detail

The contract's maxDeposit function doesn't comply with ERC-4626 which is a mentioned requirement.
According to the specification, MUST return the maximum amount of assets deposit would allow to be deposited for receiver and not cause a revert ....
The deposit function will revert in case the deposit is a lossy deposit ie. totalPreciseAsset function returns less than the totalDebt after the deposit. It is possible for this to occur due to rounding inside the preview redeem function of the yieldVault in the absence / depletion of yield buffer

POC

Add the following test inside pt-v5-vault/test/unit/PrizeVault/PrizeVault.t.sol

    function testHash_MaxDepositRevertLossy() public {
        PrizeVault testVault=  new PrizeVault(
            "PoolTogether aEthDAI Prize Token (PTaEthDAI)",
            "PTaEthDAI",
            yieldVault,
            PrizePool(address(prizePool)),
            claimer,
            address(this),
            YIELD_FEE_PERCENTAGE,
            0,
            address(this)
        );

        underlyingAsset.mint(address(yieldVault),100e18);
        YieldVault(address(yieldVault)).mint(address(100),99e18); // 99 shares , 100 assets

        uint maxDepositReturned = testVault.maxDeposit(address(this));

        uint amount_ = 99e18;
        underlyingAsset.mint(address(this),amount_);
        underlyingAsset.approve(address(testVault),amount_);

        assert(maxDepositReturned > amount_);

        vm.expectRevert();
        testVault.deposit(amount_,address(this));

    }

Impact

Failure to comply with the specification which is a mentioned necessity

Code Snippet

https://github.com/sherlock-audit/2024-05-pooltogether/blob/1aa1b8c028b659585e4c7a6b9b652fb075f86db3/pt-v5-vault/src/PrizeVault.sol#L991-L992

Tool used

Manual Review

Recommendation

Consider the yieldBuffer balance too inside the maxDeposit function

[sherlock-admin4]

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

infect3d commented:

deposit can only incur rounding issue if yield buffer is depleted but if the buffer is depleted reverts on deposit are expected__ see L112-114 of PrizeVault.sol

[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

[sherlock-admin2]

The protocol team fixed this issue in the following PRs/commits:
GenerationSoftware/pt-v5-vault#113

[10xhash]

Fixed.
Now maxDeposit returns 0 unless totalAssets is >= totalDebt + yieldBuffer/2. This will ensure that unless rounding error amounts to yieldBuffer/2, lossyDeposit will not occur

[sherlock-admin2]

The Lead Senior Watson signed off on the fix.