Frontrunning issue with updateCollateral function in the Controller
Summary
The updateCollateral function, as used in the Controller, doesn't contain validation checks for existing collaterals. This could allow for potential frontrunning, where a user deposits collateral and opens a position just before the function is run. Post the update, the Controller and its products would reference the new collateral address, effectively ignoring the position opened using the previous collateral contract.
Vulnerability Detail
The owner of the Controller has the ability to modify the collateral address using the updateCollateral function. Notably, this function lacks a validation mechanism to check if there are existing collaterals provided by any user. This presents a frontrunning opportunity. If a user were to deposit collateral and open a position right before the updateCollateral function is executed, the Controller and the products would then reference the new collateral address. The problem arises as the frontrunner has used the old collateral contract to create a position, which the product isn't aware of following the updateCollateral action.
Impact
Even tho the governance calls the updateCollateral function with precaution (checking whether there are any collateral in the previous collateral contract) it is not sufficient due to front-running of this function. There must be a validation inside the function to prevent this type of attack.
Check the optimistic ledgers total balance and require it is 0 meaning that everybody migrated their collaterals. Overall, it is a very critic function since it is changing a critic piece of the Perennial ecosystem. Collateral is an upgradable proxy so maybe it is not necessary to have this function at all?
mstpr-brainbot
medium
Frontrunning issue with updateCollateral function in the Controller
Summary
The updateCollateral function, as used in the Controller, doesn't contain validation checks for existing collaterals. This could allow for potential frontrunning, where a user deposits collateral and opens a position just before the function is run. Post the update, the Controller and its products would reference the new collateral address, effectively ignoring the position opened using the previous collateral contract.
Vulnerability Detail
The owner of the Controller has the ability to modify the collateral address using the updateCollateral function. Notably, this function lacks a validation mechanism to check if there are existing collaterals provided by any user. This presents a frontrunning opportunity. If a user were to deposit collateral and open a position right before the updateCollateral function is executed, the Controller and the products would then reference the new collateral address. The problem arises as the frontrunner has used the old collateral contract to create a position, which the product isn't aware of following the updateCollateral action.
Impact
Even tho the governance calls the updateCollateral function with precaution (checking whether there are any collateral in the previous collateral contract) it is not sufficient due to front-running of this function. There must be a validation inside the function to prevent this type of attack.
Code Snippet
https://github.com/sherlock-audit/2023-05-perennial/blob/main/perennial-mono/packages/perennial/contracts/controller/Controller.sol#L181-L185 no validation
https://github.com/sherlock-audit/2023-05-perennial/blob/main/perennial-mono/packages/perennial/contracts/product/Product.sol#L287-L313 user can create a make or take function in the front-running tx
Tool used
Manual Review
Recommendation
Check the optimistic ledgers total balance and require it is 0 meaning that everybody migrated their collaterals. Overall, it is a very critic function since it is changing a critic piece of the Perennial ecosystem. Collateral is an upgradable proxy so maybe it is not necessary to have this function at all?