code423n4 commented 1 year ago

Lines of code

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

Vulnerability details

Impact

Any users that try to call mintWithPermit will have their transaction reverted. This will drain gas unnecessarily.

Proof of Concept

The ERC20 Permit extension allows approvals to be made via signatures. The idea behind this is to only require the user to submit 1 transaction on chain and therefore only pay the gas for a single transaction rather than having to perform an approval step in another transaction.

The vault contract has 2 permit methods: mintWithPermit and depositWithPermit. With the former you specify the number of shares you want to mint whereas with the latter you specify the number of assets you want to deposit.

The issue with the mintWithPermit method is that the signature for the approval has to be signed with the value of _assets that you're approving to the contract, however the value of _assets isn't known in advance. The check for a valid signature will always fail because there isn't a public method for the user to convert between assets and shares, and most users will probably sign a message with the number of shares instead.

Tools Used

Manual review

Recommended Mitigation Steps

The mintWithPermit method should probably be removed. Even if there was a public view to convert between assets and shares, there is a chance that the exchange rate changes after the user reads the view and before the user signs the message and calls mintWithPermit.

Assessed type

ERC20

c4-judge commented 1 year ago

Picodes marked the issue as duplicate of #384