PARMinerV2's liquidate() can be run repeatedly for the same collateralToken with different arguments.
For example, different Vaults can have the same token, so there can be unrelated runs with different vaultId / DEX data, but the same collateralToken and proxy.
If collateralToken use approval race protection, say it's USDT, then liquidate can become stuck and unavailable for any calls with USDT and the proxy.
This can happen because previous liquidate() runs are not guaranteed to use up exactly the whole balance allowance given, so there can remain some positive allowance for the same proxy (dust or something substantial doesn't matter), and collateralToken.approve(proxy, collateralToken.balanceOf(address(this))) call will fail in such a case. Some manual fix to be needed (empty the balance, craft the params, rerun liquidate so it approve zero) each time this happens.
Setting severity to medium as this clearly impacts liquidate() availability to the users.
Proof of Concept
liquidate approves proxy for the whole collateralToken balance of the contract, while subsequent router call isn't guaranteed to use it all:
gzeoneth
commented
2 years ago
Lines of code
https://github.com/code-423n4/2022-04-mimo/blob/b18670f44d595483df2c0f76d1c57a7bfbfbc083/core/contracts/liquidityMining/v2/PARMinerV2.sol#L125-L126
Vulnerability details
PARMinerV2's liquidate() can be run repeatedly for the same collateralToken with different arguments.
For example, different Vaults can have the same token, so there can be unrelated runs with different vaultId / DEX data, but the same collateralToken and proxy.
If collateralToken use approval race protection, say it's USDT, then liquidate can become stuck and unavailable for any calls with USDT and the proxy.
This can happen because previous liquidate() runs are not guaranteed to use up exactly the whole balance allowance given, so there can remain some positive allowance for the same proxy (dust or something substantial doesn't matter), and
collateralToken.approve(proxy, collateralToken.balanceOf(address(this)))call will fail in such a case. Some manual fix to be needed (empty the balance, craft the params, rerun liquidate so it approve zero) each time this happens.Setting severity to medium as this clearly impacts liquidate() availability to the users.
Proof of Concept
liquidate approves proxy for the whole collateralToken balance of the contract, while subsequent router call isn't guaranteed to use it all:
https://github.com/code-423n4/2022-04-mimo/blob/b18670f44d595483df2c0f76d1c57a7bfbfbc083/core/contracts/liquidityMining/v2/PARMinerV2.sol#L125-L126
collateralToken can be arbitrary, while some ERC20 tokens do not allow an approval of a positive value if allowance is positive already:
https://github.com/d-xo/weird-erc20#approval-race-protections
Recommended Mitigation Steps
Consider adding zero amount approval before actual amount approval, i.e. force zero allowance before current approval.