usmannk - Attackers can force surge to never update the collateralization ratio
github-actions opened this issue · 4 comments
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.
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
Fixed
Fix looks good. Requires _collateralRatioFallDuration and _collateralRatioRecoveryDuration be less than _maxCollateralRatioMantissa.