PoolTogether - Tripathi's results

Lines of code

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

Vulnerability details

Summary

PrizeVault::claimYieldFeeShares Transfers yield fee shares to the yield fee recipient. instead of decreasing transferred shares it resets the yieldFeeBalance even for partial shares

Impact

yieldFeeBalance will lost due to incorrect update

Proof of Concept

    /// @notice Transfers yield fee shares to the yield fee recipient
    /// @param _shares The shares to mint to the yield fee recipient
    /// @dev Emits a `ClaimYieldFeeShares` event
    /// @dev Will revert if the caller is not the yield fee recipient or if zero shares are withdrawn
    function claimYieldFeeShares(uint256 _shares) external onlyYieldFeeRecipient {
        if (_shares == 0) revert MintZeroShares();

        uint256 _yieldFeeBalance = yieldFeeBalance;
        if (_shares > _yieldFeeBalance) revert SharesExceedsYieldFeeBalance(_shares, _yieldFeeBalance);

        yieldFeeBalance -= _yieldFeeBalance;
        //@audit-issue it should be yieldFeeBalance -= shares

        _mint(msg.sender, _shares);

        emit ClaimYieldFeeShares(msg.sender, _shares);
    }

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

Transferring _shares should decrease yieldFeeBalance by _shares, while it resets the yieldFeeBalance to zero

Tools Used

Manual

Recommended Mitigation Steps

-     yieldFeeBalance -= _yieldFeeBalance;
+     yieldFeeBalance -= _shares;

Assessed type

Math

Lines of code

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

Vulnerability details

Summary

prizeVault::withdraw() is used to withdraw exact number of assets by buring shares and sent to receiver.

But this function lacks maxShares burnt param, without this caller could burn more than expected amount of shares for corresponding assets.

Impact

caller will end up burning more shares than he expect

Proof of Concept

    function withdraw(
        uint256 _assets,
        address _receiver,
        address _owner
    ) external returns (uint256) {
        uint256 _shares = previewWithdraw(_assets);
        _burnAndWithdraw(msg.sender, _receiver, _owner, _shares, _assets);
        return _shares;
    }

https://github.com/code-423n4/2024-03-pooltogether/blob/main/pt-v5-vault/src/PrizeVault.sol#L489 withdraw only takes _assets the amount of underlying assets to be withdrawn by burning whatever amount of shares which is not an optimised trade for caller.

In many case caller could end up burning more than he expected

eg. before submitting transaction Alice see that everything is fine so she submitted a tx for withdrawing assets due to some malicious actions or inconsistent behaviour vault goes into recovery mode i.e totalAssets()< totalDebt()

Now 1 share corresponds to less than 1 asset but Alice tx will be executed as normal tx and she will end up burning more shares than she expected

Tools Used

Manual

Recommended Mitigation Steps

Add proper slippage checks during withdrawing

Assessed type

Other

1 PrizeVault->Claimable->HookManager::setHooks() allows Unintended or Malicious Use of Prize Winners' Hook

The setHooks function in Vault.sol allows users to set arbitrary hooks, potentially enabling them to make external calls with unintended consequences. This vulnerability could lead to various unexpected behaviors, such as unauthorized side transactions with gas paid unbeknownst to the claimer, reentrant calls, or denial-of-service attacks on claiming transactions.

 function setHooks(VaultHooks calldata hooks) external {
        _hooks[msg.sender] = hooks;
        emit SetHooks(msg.sender, hooks);
    }

https://github.com/code-423n4/2024-03-pooltogether/blob/main/pt-v5-vault/src/abstract/HookManager.sol#L29 it is possible for a hook to make an external call and modify the EVM state.

Handle these malicious calls

2 PrizeVaultFactory::deployVault allows deployment of vaults with non-authentic claimer, prizePool etc

A malicious vault can be deployed via the official PrizeVaultFactory contract. Users may lose funds while interacting with such vaults.

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

ensure proper validation or implement kind of trusted role for the deployement of new vaults

3 PrizeVault::setYieldFeeRecipient doesn't check for zero address

    function setYieldFeeRecipient(address _yieldFeeRecipient) external onlyOwner {
        _setYieldFeeRecipient(_yieldFeeRecipient);
    }
    function _setYieldFeeRecipient(address _yieldFeeRecipient) internal {
        //@audit lacks zero address check
        yieldFeeRecipient = _yieldFeeRecipient;
        emit YieldFeeRecipientSet(_yieldFeeRecipient);
    }

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

4 depositWithPermit doesn't server the purpose of permit function since even after sign the tx only signer can call this function

    function depositWithPermit(
        uint256 _assets,
        address _owner,
        uint256 _deadline,
        uint8 _v,
        bytes32 _r,
        bytes32 _s
    ) external returns (uint256) {
        if (_owner != msg.sender) {
            revert PermitCallerNotOwner(msg.sender, _owner);
        }

        // Skip the permit call if the allowance has already been set to exactly what is needed. This prevents
        // griefing attacks where the signature is used by another actor to complete the permit before this
        // function is executed.
        if (_asset.allowance(_owner, address(this)) != _assets) {
            IERC20Permit(address(_asset)).permit(_owner, address(this), _assets, _deadline, _v, _r, _s);/
        }

        uint256 _shares = previewDeposit(_assets);
        _depositAndMint(_owner, _owner, _assets, _shares);
        return _shares;
    }

https://github.com/code-423n4/2024-03-pooltogether/blob/main/pt-v5-vault/src/PrizeVault.sol#L532 fix this so that it serves the purpose of permit function or remove the function to save deployment cost

5 depositWithPermit lacks chainID which allows signatures to be re-used across forks

    function depositWithPermit(
        uint256 _assets,
        address _owner,
        uint256 _deadline,
        uint8 _v,
        bytes32 _r,
        bytes32 _s
    ) external returns (uint256) {
///////////////////////
        if (_asset.allowance(_owner, address(this)) != _assets) {
            IERC20Permit(address(_asset)).permit(_owner, address(this), _assets, _deadline, _v, _r, _s);/
        }
///////////////////////
    }

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

Scenario :: An EIP is included in an upcoming hard fork that has split the community. After the hard fork, a significant user base remains on the old chain. On the new chain, Alice approves some tokens via a call to permit. Alice, operating on both chains, replays the permit call on the old chain

Include the chainID opcode in the permit schema. This will make replay attacks impossible in the event of a post-deployment hard fork.