code423n4 commented 1 year ago

Lines of code

https://github.com/GenerationSoftware/pt-v5-vault/blob/b1deb5d494c25f885c34c83f014c8a855c5e2749/src/Vault.sol#L864-L874 https://github.com/GenerationSoftware/pt-v5-vault/blob/b1deb5d494c25f885c34c83f014c8a855c5e2749/src/Vault.sol#L882-L887

Vulnerability details

Impact

The EIP-4626 states that for convertToShares and convertToAssets function MUST NOT be inclusive of any fees that are charged against assets in the Vault as can be seen in the EIP-4626 file https://eips.ethereum.org/EIPS/eip-4626, but in the case where the Vault is undercollateralized the calculation of _currentExchangeRate takes into account any unclaimed yield fee https://github.com/GenerationSoftware/pt-v5-vault/blob/b1deb5d494c25f885c34c83f014c8a855c5e2749/src/Vault.sol#L1163-L1164 which makes convertToShares and convertToAssets who use _currentExchangeRate every time they are called not compliant to EIP-4626 for this case.

Proof of Concept

Per security considerations that are written in the EIP-4626 :

The methods totalAssets, convertToShares and convertToAssets are estimates useful for display purposes, and do not have to confer the exact amount of underlying assets their context suggests
The preview methods return values that are as close as possible to exact as possible. For that reason, they are manipulable by altering the on-chain conditions and are not always safe to be used as price oracles. This specification includes convert methods that are allowed to be inexact and therefore can be implemented as robust price oracles. For example, it would be correct to implement the convert methods as using a time-weighted average price in converting between assets and shares

which implies that in a case of a EIP-4626 compliant protocol you can implement a robust TWAP oracle based on the convertToAssets and convertToShares functions, if those methods respects the EIP-4626 standard. In the case of how the ERC4626 it is implemented in the Vault.sol other protocols could assume full compliancy with EIP-4626 and use convertToAssets or convertToShares to build a TWAP oracle or anything similar, but in the case of the a Vault being undercollateralized the results will different then the one expected which could hurt anybody interacting with it.

Tools Used

Manual review

Recommended Mitigation Steps

Consider changing the way _convertToShares and _convertToAssets are calculated to be compliant with EIP-4626 in any case or specify the fact that for this particular case these methods are not fully compliant so other people/protocol will take into account that.

Assessed type

ERC4626

asselstine commented 1 year ago

The 4626 spec in question is that [convertToShares] MUST NOT be inclusive of any fees that are charged against assets in the Vault

So convertToShares should not include fees. This is always the case! The Vault tranches the fees as if the deposits were the senior tranche and the fees are the junior tranche. If the Vault becomes under-collateralized then the junior tranche (fees) are lowered to cover the senior tranche (deposits).

Here, we're using the fee in the calculation because it's essentially reducing the fee to cover the collateral. It's not inclusive of the fee; it's simply reducing the fee so that users get their full deposits back.

Unless I'm missing something?