search

sherlock-audit / 2024-05-pooltogether-judging

2 stars 0 forks source link

hash - `maxDeposit` doesn't comply with ERC-4626 #134

Open sherlock-admin2 opened 1 month ago

sherlock-admin2 commented 1 month ago

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 commented 1 month ago

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 commented 1 month ago

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 commented 3 weeks ago

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

10xhash commented 1 week ago

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 commented 1 week ago

The Lead Senior Watson signed off on the fix.