Potential DOS in `removeCollateral`

[code423n4]

Lines of code

https://github.com/with-backed/papr/blob/9528f2711ff0c1522076b9f93fba13f88d5bd5e6/src/PaprController.sol#L109-L135 https://github.com/with-backed/papr/blob/9528f2711ff0c1522076b9f93fba13f88d5bd5e6/src/PaprController.sol#L444 https://github.com/with-backed/papr/blob/9528f2711ff0c1522076b9f93fba13f88d5bd5e6/src/PaprController.sol#L118-L134

Vulnerability details

Impact

Function removeCollateral may fail under certain circumstances, potentially causing DOS to user trying to withdraw their collateral asset. This action may be time critical and may cause the user to lose funds due to price change etc.

Proof of Concept

https://swcregistry.io/docs/SWC-113

Calls to external contracts inside a loop are dangerous (especially if the loop index can be user-controlled).

If the array collateralArr is large it may cause the loop below to run out of gas or one of the transfer calls on Line 444 fails which isn't accounted for.

https://github.com/with-backed/papr/blob/9528f2711ff0c1522076b9f93fba13f88d5bd5e6/src/PaprController.sol#L118-L134 https://github.com/with-backed/papr/blob/9528f2711ff0c1522076b9f93fba13f88d5bd5e6/src/PaprController.sol#L444

Tools Used

Manual Audit

Recommended Mitigation Steps

Avoid combining multiple calls in a single transaction, especially when calls are executed as part of a loop. Always assume that external calls can fail. Implement the contract logic to handle failed calls.

[c4-judge]

trust1995 marked the issue as unsatisfactory: Insufficient quality