code423n4 commented 1 year ago

Lines of code

https://github.com/GenerationSoftware/pt-v5-vault/blob/b1deb5d494c25f885c34c83f014c8a855c5e2749/src/Vault.sol#L407-L415

Vulnerability details

Impact

deposit/mint in Vault does not allow expression of minimum acceptable output for user leading to potential principle loss from user due to MEV.

In the specification of ERC4626 yield-bearing token, shares and underlying (asset) are used to represent the ownership and assets of the vault respectively, convertToShares and convertToAssets are methods to calculate the conversion ratio from one to another. Implicitly, a vault aim to increase the asset per share through custom strategy under-the-hook, however at times of volatility it would also result in a reduction of asset/share when the accounting of losses are made.

Now, when we examine the Vault (PoolTogether Vault/PT), that sits on top of the underlying yield vault, the deposit, does not allow users to express the minimum acceptable share(s) during the deposit.

  function deposit(uint256 _assets, address _receiver) public virtual override returns (uint256) {
    if (_assets > maxDeposit(_receiver))
      revert DepositMoreThanMax(_receiver, _assets, maxDeposit(_receiver));

    uint256 _shares = _convertToShares(_assets, Math.Rounding.Down);
    _deposit(msg.sender, _receiver, _assets, _shares);

    return _shares;
  }

https://github.com/GenerationSoftware/pt-v5-vault/blob/b1deb5d494c25f885c34c83f014c8a855c5e2749/src/Vault.sol#L407-L415C4

Since the deposit would in turn deposit assets into the underlying yield vault, which would also likely conduct some market operation(s) to put the new fund in operation, this trade would cause slippage.

Impact: user who deposit from PoolTogether vault might be subject to MEV attack since they in turns interact with the underlying vault without slippage protection.

Proof of Concept

Tools Used

Recommended Mitigation Steps

add a parameter to allow caller to specify the minimum required shares to accept the deposit execution

Assessed type

MEV

c4-judge commented 1 year ago

Picodes marked the issue as primary issue

c4-sponsor commented 1 year ago

asselstine requested judge review

asselstine commented 1 year ago

I find the explanation confusing; the warden mentions underlying yield vault slippage as being the problem, then says MEV is an issue.

I need more context here, an example would be helpful if there is a problem?

PierrickGT commented 1 year ago

This kind of addition wouldn't be compliant with the EIP-4626 spec, since the deposit function only accepts two parameters, assets and receiver: https://eips.ethereum.org/EIPS/eip-4626#deposit

If the user wants to know how many shares they will receive for depositing any amount of assets, they can call the previewDeposit function and revert if the amount of shares they will receive doesn't meet their criteria: https://eips.ethereum.org/EIPS/eip-4626#previewdeposit

Picodes commented 1 year ago

I do agree with @PierrickGT's comment above. The EIP doesn't specify a slippage and it is useless for most ERC4626. Also this could eventually be handled by a router when applicable.