Deposit transaction is prone to being front-run by bad actors.

[code423n4]

Lines of code

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

Vulnerability details

Impact

It is possible for an attacker to front-run a user's deposit transaction while transferring fewer amount of assets than the user and minting an equivalent amount of shares as the user could have.

Proof of Concept

The scenario described below is for the deposit function in the Vault contract.

Consider this scenario with Party A and Party B when _vaultAssets are not 0. Party B calls the deposit function such that the asset amount satisfies the condition:

_assets > _vaultAssets, where _vaultAssets is the balance of the vault for _asset token.

This means that when the transaction gets executed the asset amount that is transferred from party B is:

_assetsDeposit = _assets - _vaultAssets;.

Then the shares are minted based on the value of _assets to party B.

But, this design is prone to front-running. If party A is a malicious actor, they would see party B's transaction and send a similar transaction with similar inputs with higher gas fees to get their transaction executed first.

This would mean that party A would only be transferring _assetsDeposit = _assets - _vaultAssets; from their address. And after this transaction, _vaultAssets would be 0 as they get transferred to the yield vault. Then party B's transaction executes after party A's. Since _vaultAssets are 0, the amount that will be transferred from party B will be equal to the _assets in the input which is more than the amount that was transferred from party A (_assetsDeposit = _assets - _vaultAssets;).

What is important to note is that party B could be paying a higher amount of assets (for almost an equivalent amount of shares) than party A, which is unfair to party B. This scenario is significant when the exchange rate for party B does not deviate much after party A's transaction.

Tools Used

Manual Review

Recommended Mitigation Steps

It is recommended that the protocol does not implement the deposit function in this manner. Assets supplied in the input should always be transferred from the user and not the _assetsDeposit amount.

Or if the protocol wants to continue using _assetsDeposit, then the protocol should mint shares which are calculated from _assetsDeposit rather than _assets. The convertToShares function should take _assetsDeposit as the input, not _assets.

Assessed type

MEV

[c4-judge]

Picodes marked the issue as unsatisfactory: Invalid

[Picodes]

The contract is not suppose to hold any funds