Reentrancy issue in Vault.deposit can lead to drain all funds of vault if _asset is ERC777 token

[code423n4]

Lines of code

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

Vulnerability details

In deposit function, developer conscious of reentrancy possibility when using ERC777 token and use CEI pattern to avoid impact of reentrancy:

// If _asset is ERC777, `transferFrom` can trigger a reentrancy BEFORE the transfer happens through the
// `tokensToSend` hook. On the other hand, the `tokenReceived` hook that is triggered after the transfer
// calls the vault which is assumed to not be malicious.
//
// Conclusion: we need to do the transfer before we mint so that any reentrancy would happen before the
// assets are transferred and before the shares are minted, which is a valid state.

// We only need to deposit new assets if there is not enough assets in the vault to fulfill the deposit`

But it is not enough to reduce impact of reentrancy. Attacker can still abusing this to steal all funds. Root cause come from share calculation:

uint256 _shares = _convertToShares(_assets, Math.Rounding.Down);

When attacker re-enter function while excute function, _mint() function still not executed, so function _updateExchangeRate is not excuted too. Attacker can get more shares by using old share calculation rate. Repeat this process till all vault is drained.

Impact

All vault is drained.

Tools Used

Manual review.

Recommended Mitigation Steps

Using ReentrancyGuard from OpenZeppelin

Assessed type

Reentrancy

[c4-judge]

Picodes marked the issue as unsatisfactory: Invalid

[Picodes]

_convertToShares hasn't changed at the time of the reentrancy as assets have not been deposited in the yield vault