`setLiquidationPair` in `Vault.sol` can revert 100% in some cases which makes changing `_liquidationPair` impossible

[code423n4]

Lines of code

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

Vulnerability details

Impact

The function setLiquidationPair is used to set the _liquidationPair by the owner, which is the only address that is able to call liquidate. The functions checks if the _previousLiquidationPair is address 0 and if that is not the case, it will reset the allowance of that address to 0 as can be seen here https://github.com/GenerationSoftware/pt-v5-vault/blob/b1deb5d494c25f885c34c83f014c8a855c5e2749/src/Vault.sol#L674 The problem relies in the fact that there are some ERC20 tokens that revert if the value of transfer, transferFrom and approve is 0, one very big example of this is BNB, one of the ERC20's with the largest market caps which is wieldy used.

Proof of Concept

An user creates a Vault, calling deployVault in VaultFactory.sol with the asset token being BNB. The _liquidationPair is set to be a singleton contract per-chain ,as it is stated in the description of the project. This singleton contract, that manages every liquidate calls on all the vaults, gets redeployed or gets upgraded and all of the Vaults needs their _liquidationPair updated as well. The owners who has BNB as their asset token will try to call setLiquidationPair but this will revert all the time because the function will try to approve to 0 the _previousLiquidationPair, thus reverting all the time https://etherscan.io/token/0xB8c77482e45F1F44dE1745F52C74426C631bDD52#code#L94 . This will make updating the _liquidationPair for those contracts impossible, which also makes the liquidate impossible to be called.

Tools Used

Manual review

Recommended Mitigation Steps

Consider specify to the user that BNB or any token that revert on 0 values are not able to interact with the protocol as intended or don't try to approve to 0 _previousLiquidationPair and just approve the new _liquidationPair with the amount needed.

Assessed type

ERC20

[c4-sponsor]

asselstine marked the issue as sponsor confirmed

[Picodes]

From https://eips.ethereum.org/EIPS/eip-20: "NOTE: To prevent attack vectors like the one described here and discussed here, clients SHOULD make sure to create user interfaces in such a way that they set the allowance first to 0 before setting it to another value for the same spender. THOUGH The contract itself shouldn’t enforce it, to allow backwards compatibility with contracts deployed before"

From this I think we can consider that not allowing resetting allowances to 0 is a bug in the ERC20 token and not in this vault contract. Downgrading to Low for this reason.

[c4-judge]

Picodes changed the severity to QA (Quality Assurance)

[c4-judge]

Picodes marked the issue as grade-b

[PierrickGT]

The LiquidationPair actually don't need to move any assets, so we can remove this approval. Removed in this PR: https://github.com/GenerationSoftware/pt-v5-vault/pull/26