sherlock-audit / 2023-05-USSD-judging

9 stars 7 forks source link

neumo - If collateral factor is high enough, flutter ends up being out of bounds #889

Open sherlock-admin opened 1 year ago

sherlock-admin commented 1 year ago

neumo

high

If collateral factor is high enough, flutter ends up being out of bounds

Summary

In USSDRebalancer contract, function SellUSSDBuyCollateral will revert everytime a rebalance calls it, provided the collateral factor is greater than all the elements of the flutterRatios array.

Vulnerability Detail

Function SellUSSDBuyCollateral calculates flutter as the lowest index of the flutterRatios array for which the collateral factor is smaller than the flutter ratio.

uint256 cf = IUSSD(USSD).collateralFactor();
uint256 flutter = 0;
for (flutter = 0; flutter < flutterRatios.length; flutter++) {
    if (cf < flutterRatios[flutter]) {
      break;
    }
}

The problem arises when, if collateral factor is greater than all flutter values, after the loop flutter = flutterRatios.length.

This flutter value is used afterwards here:

...
if (collateralval * 1e18 / ownval < collateral[i].ratios[flutter]) {
  portions++;
}
...

And here:

...
if (collateralval * 1e18 / ownval < collateral[i].ratios[flutter]) {
  if (collateral[i].token != uniPool.token0() || collateral[i].token != uniPool.token1()) {
    // don't touch DAI if it's needed to be bought (it's already bought)
    IUSSD(USSD).UniV3SwapInput(collateral[i].pathbuy, daibought/portions);
  }
}
...

As we can see in the tests of the project, the flutterRatios array and the collateral ratios array are set to be of the same length, so if flutter = flutterRatios.length, any call to that index in the ratios array will revert with an index out of bounds.

Impact

High, when the collateral factor reaches certain level, a rebalance that calls SellUSSDBuyCollateral will always revert.

Code Snippet

https://github.com/sherlock-audit/2023-05-USSD/blob/main/ussd-contracts/contracts/USSDRebalancer.sol#L178-L184

Tool used

Manual review.

Recommendation

When checking collateral[i].ratios[flutter] always check first that flutter is < flutterRatios.length.

neumoxx commented 1 year ago

Escalate for 10 USDC The issue is marked as duplicate of #940, and in that issue there's a comment from the judge that states This is an admin input, requires admin error to cause problems. The issue does not depend on an admin input to arise. The flutter ratios are set in the tests according to values mentioned in the whitepaper: https://github.com/sherlock-audit/2023-05-USSD/blob/6d7a9fdfb1f1ed838632c25b6e1b01748d0bafda/ussd-contracts/test/USSDsimulator.test.js#L391-L393. The collateral factor: https://github.com/sherlock-audit/2023-05-USSD/blob/6d7a9fdfb1f1ed838632c25b6e1b01748d0bafda/ussd-contracts/contracts/USSD.sol#L179-L191 can grow beyond the last value of the flutter ratios array and that would make the SellUSSDBuyCollateral function to revert.

sherlock-admin commented 1 year ago

Escalate for 10 USDC The issue is marked as duplicate of #940, and in that issue there's a comment from the judge that states This is an admin input, requires admin error to cause problems. The issue does not depend on an admin input to arise. The flutter ratios are set in the tests according to values mentioned in the whitepaper: https://github.com/sherlock-audit/2023-05-USSD/blob/6d7a9fdfb1f1ed838632c25b6e1b01748d0bafda/ussd-contracts/test/USSDsimulator.test.js#L391-L393. The collateral factor: https://github.com/sherlock-audit/2023-05-USSD/blob/6d7a9fdfb1f1ed838632c25b6e1b01748d0bafda/ussd-contracts/contracts/USSD.sol#L179-L191 can grow beyond the last value of the flutter ratios array and that would make the SellUSSDBuyCollateral function to revert.

You've created a valid escalation for 10 USDC!

To remove the escalation from consideration: Delete your comment.

You may delete or edit your escalation comment anytime before the 48-hour escalation window closes. After that, the escalation becomes final.

ctf-sec commented 1 year ago

here

    function setFlutterRatios(uint256[] calldata _flutterRatios) public onlyControl {
      flutterRatios = _flutterRatios;
    }

flutterRatios can be adjusted by admin

Valid low

hrishibhat commented 1 year ago

Result: Medium Has duplicates This is a valid issue where the rebalance reverts in certain conditions due to the unexpected final loop flutter values.

sherlock-admin commented 1 year ago

Escalations have been resolved successfully!

Escalation status: