Malicious user can steal other user's deposits from Vault.sol

[code423n4]

Lines of code

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

Vulnerability details

Impact

When the Vault.withdraw() function is called, a maximum of type(uint96).max shares are being burnt subsequently: Vault.withdraw()-> Vault._withdraw()-> Vault._burn burns uint96(_shares), see Vault.sol line 1139.

A malicious user can exploit this in the following way:

1. A malicious user deposits for example two times a value of type(uint96).max underlying assets into the Vault, calling two times the function Vault.deposit(). They can't deposit more in a single transaction because type(uint96).max is the maximum value to deposit.

2. Then the malicious user calls Vault.withdraw() with a higher value of assets to withdraw than type(uint96).max, for example they withdraw (2 * type(uint96).max) which is the total amount of assets they depositted before.

3. Now what happens is that the Vault.sol contract only burns type(uint96).max shares for the user, but transfers 2 * type(uint96).max underlying assets to the malicious user, which is the total amount they depositted before.

4. This happens because Vault._burn() only burns uint96(shares) shares of the malicious users - see Vault.sol line 1155.

5. Now the malicious user has still vault shares left but they withdrew the total amount of their depositted assets.

6. Now the vault transferred the total amount of the malicious user's assets back to them, and the malicious user has still shares left to withdraw even more assets that are now being stolen from assets depositted by other users.

7. Or if the malicious user was the first depositor, they wait until another user deposits and the malicious user can now withdraw the other users depositted assets since the malicious user still has Vault shares left.

8. Or if the malicious user is not the first depositor, they use a flashLoan or flashMint to deposit multiple times type(uint96).max assets into the vault, then withdraw their deposit, pay back the flashLoan or flashMint and they will still have enough vault shares left to steal all other users assets by withdrawing them.

In this way, other user's depositted assets can be stolen, as explained above.

Proof of Concept

Here is a POC, where the problem is illustrated:

https://gist.github.com/zzzitron/397790302ca95aa3fbf05694ae1497ab

Tools Used

Manual review

Recommended Mitigation Steps

Consider adjusting the Vault._burn function to not convert from uint256 to uint96 when burning shares.

Assessed type

Math

[c4-sponsor]

asselstine marked the issue as sponsor confirmed

[PierrickGT]

Fixed in this PR by using SafeCast: GenerationSoftware/pt-v5-vault#9

[c4-judge]

Picodes marked the issue as satisfactory

[c4-judge]

Picodes marked the issue as primary issue

[c4-judge]

Picodes marked the issue as selected for report