Vault.withdraw() and Vault.redeem() doesn't conform to EIP-4626, might cause integration problems in the future, that can lead to a wide range of issues for parties involved, including loss of funds.

[code423n4]

Lines of code

https://github.com/GenerationSoftware/pt-v5-vault/blob/b1deb5d494c25f885c34c83f014c8a855c5e2749/src/Vault.sol#L509 https://github.com/GenerationSoftware/pt-v5-vault/blob/b1deb5d494c25f885c34c83f014c8a855c5e2749/src/Vault.sol#L524 https://github.com/GenerationSoftware/pt-v5-vault/blob/b1deb5d494c25f885c34c83f014c8a855c5e2749/src/Vault.sol#L1007

Vulnerability details

Impact

Vault.withdraw() and Vault.redeem() doesn't conform to EIP-4626 standard

Contracts that integrate with Vault.sol (including poolTogether contracts) may wrongly assume that the functions are EIP-4626 compliant, which it might cause integration problems in the future, that can lead to a wide range of issues for both parties, including loss of funds.

Proof of Concept

According to EIP-4626 standard for withdrawals "withdrawal process MUST revert if all of assets cannot be withdrawn (due to withdrawal limit being reached, slippage, the owner not having enough shares, etc)"

This PoC describes this issue for the withdraw function, but there is the same problem with the redeem function.

EIP-4626 specification says that the withdraw function:

// Burns shares from owner and sends exactly assets of underlying tokens to receiver.

// MUST revert if all of assets cannot be withdrawn (due to withdrawal limit being reached, slippage,
// the owner not having enough shares, etc).

But checking Vault.withdraw() and Vault.redeem() it is clear that they don't conform to this, as there are no checks to ensure revert if all assets can't be withdrawn due to slippage e.t.c

Tools Used

Manual Review

Recommended Mitigation Steps

Rebuild Vault.sol to conform to EIP-4626 standard for withdrawals

Assessed type

Other

[Picodes]

But checking Vault.withdraw() and Vault.redeem() it is clear that they don't conform to this -> Please prove your assumption, especially when they are the core of the report.

[c4-judge]

Picodes marked the issue as unsatisfactory: Insufficient quality