c4-bot-10 commented 5 months ago

Lines of code

https://github.com/code-423n4/2024-03-pooltogether/blob/main/pt-v5-vault/src/PrizeVault.sol#L441-L450

Vulnerability details

Impact

When _totalAssets < totalDebt_, shares and assets are not 1:1, but previewDeposit and previewMint still work as they are 1:1, and result in incorrect values. The impact is that user's withdraws will revert, as it tends to burn more shares than minted.

Proof of Concept

As shown in previewDeposit and previewMint, shares and assets are always 1:1.

441:    function previewDeposit(uint256 _assets) public pure returns (uint256) {
442:        // shares represent how many assets an account has deposited, so they are 1:1 on deposit
443:=>      return _assets;
444:    }

447:    function previewMint(uint256 _shares) public pure returns (uint256) {
448:        // shares represent how many assets an account has deposited, so they are 1:1 on mint
449:=>      return _shares;
450:    }

https://github.com/code-423n4/2024-03-pooltogether/blob/main/pt-v5-vault/src/PrizeVault.sol#L441-L450

But according to previewWithdraw, shares and assets are not always 1:1. When _totalAssets < totalDebt_, one asset will return more shares.

454:    function previewWithdraw(uint256 _assets) public view returns (uint256) {
455:        uint256 _totalAssets = totalAssets();
456:
457:        // No withdrawals can occur if the vault controls no assets.
458:        if (_totalAssets == 0) revert ZeroTotalAssets();
459:
460:        uint256 totalDebt_ = totalDebt();
461:        if (_totalAssets >= totalDebt_) {
462:            return _assets;
463:        } else {
464:            // Follows the inverse conversion of `convertToAssets`
465:=>          return _assets.mulDiv(totalDebt_, _totalAssets, Math.Rounding.Up);
466:        }
467:    }

https://github.com/code-423n4/2024-03-pooltogether/blob/main/pt-v5-vault/src/PrizeVault.sol#L454-L467

Let's assume that _totalAssets < totalDebt_ has occurred, and assume _totalAssets = 100, totalDebt_ = 110 for simplicity. Consider the following scenario:

Alice deposits 100 assets, and get 100 shares minted (1:1 according to previewDeposit)
Alice withdraws her 100 assets immediately afterwards (ignore the yield change), the shares needed to burn is 100*210/200=105 (according to previewWithdraw). As long as the assets and shares is not 1:1, Alice cannot withdraw her all assets.

Tools Used

VSCode

Recommended Mitigation Steps

    function previewDeposit(uint256 _assets) public pure returns (uint256) {
        // shares represent how many assets an account has deposited, so they are 1:1 on deposit
-        return _assets;
+        return convertToShares(_assets);
    }

    function previewMint(uint256 _shares) public pure returns (uint256) {
        // shares represent how many assets an account has deposited, so they are 1:1 on mint
-       return _shares;
+       return convertToAssets(_shares);
    }