usmannk - Attackers can force surge to never update the collateralization ratio

[github-actions]

usmannk

high

Attackers can force surge to never update the collateralization ratio

Summary

Certain parameter choices make it feasible to block updates to the collateralization ratio. Collaterization ratio updates are calculated as uint change = timeDelta * _maxCollateralRatioMantissa / _collateralRatioRecoveryDuration; . However, with quick refreshes or a _collateralRatioRecoveryDuration that is greater than _maxCollateralRatioMantissa, this change may be zero every iteration.

Vulnerability Detail

https://github.com/sherlock-audit/2023-02-surge/blob/main/surge-protocol-v1/src/Pool.sol#L216-L263

The getCollateralRatioMantissa function calculates the collateralization ratio by linearly updating along _maxCollateralRatioMantissa / _collateralRatioRecoveryDuration. However, these updates may be forced to zero in certain situations.

Consider a pool where the loan token is WBTC and the collateral token is DAI. Given a BTC price of $20,000 it is reasonable to only allow 1/10000 BTC to be borrowed per DAI (for a max rate of $10,000 per BTC).

The _maxCollateralRatioMantissa in this case would be 1e14. In the Surge tests, a _collateralRatioRecoveryDuration of 1e15 is used. If an attacker does a tiny deposit of 1wei WBTC more often than once every 10 seconds, the change of the max collateralization ratio will always be zero no matter what the current utilization is because (timeDelta * _maxCollateralRatioMantissa) is less than _collateralRatioRecoveryDuration.

This would halt the entire adaptive pricing scheme of the Surge protocol while still allowing borrows at the current rate.

The README specifies that Surge is meant to be deployed on DEPLOYMENT: Mainnet, Optimism, Arbitrum, Fantom, Avalanche, Polygon, BNB Chain and other EVM chains. This exploit is especially attractive on L2s because of cheap/free execution (e.g. Optimism) and very low block times (thus low timeDelta).

Impact

Loss of funds for depositors as the price becomes stale and the collateralization rate, and thus pool exchange rate, of the Surge pool would no longer update.

Code Snippet

Tool used

Manual Review

Recommendation

Ensure that _collateralRatioRecoveryDuration < _maxCollateralRatioMantissa. This would preclude some pools from existing, but save funds from being stolen.

[hrishibhat]

Given the unlikely edge case of having a pool with an edge case mentioned by the Sponsor:

a legit pool might have recovery duration set to max uint in case lenders wouldn't want the collateral factor to ever rise back up after falling

Considering this issue as a valid medium

[0xMoaz]

Fixed

[0xMoaz]

Surge-fi/surge-protocol-v1@fc0fb9f

[IAm0x52]

Fix looks good. Requires _collateralRatioFallDuration and _collateralRatioRecoveryDuration be less than _maxCollateralRatioMantissa.