Skip to content
This repository has been archived by the owner on Nov 26, 2023. It is now read-only.

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

Open
sherlock-admin opened this issue May 24, 2023 · 5 comments
Labels
Escalation Resolved This issue's escalations have been approved/rejected Has Duplicates A valid issue with 1+ other issues describing the same vulnerability Medium A valid Medium severity issue Reward A payout will be made for this issue

Comments

@sherlock-admin
Copy link
Contributor

sherlock-admin commented May 24, 2023

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.

@github-actions github-actions bot closed this as completed Jun 5, 2023
@github-actions github-actions bot added Medium A valid Medium severity issue Duplicate A valid issue that is a duplicate of an issue with `Has Duplicates` label labels Jun 5, 2023
@sherlock-admin sherlock-admin added Non-Reward This issue will not receive a payout and removed Medium A valid Medium severity issue Duplicate A valid issue that is a duplicate of an issue with `Has Duplicates` label labels Jun 23, 2023
@neumoxx
Copy link

neumoxx commented Jun 24, 2023

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
Copy link
Contributor Author

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.

@sherlock-admin sherlock-admin added the Escalated This issue contains a pending escalation label Jun 24, 2023
@ctf-sec
Copy link
Collaborator

ctf-sec commented Jun 29, 2023

here

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

flutterRatios can be adjusted by admin

Valid low

@hrishibhat
Copy link
Contributor

hrishibhat commented Jul 7, 2023

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

@hrishibhat hrishibhat reopened this Jul 7, 2023
@sherlock-admin
Copy link
Contributor Author

sherlock-admin commented Jul 7, 2023

Escalations have been resolved successfully!

Escalation status:

@sherlock-admin sherlock-admin added Reward A payout will be made for this issue and removed Non-Reward This issue will not receive a payout labels Jul 7, 2023
@hrishibhat hrishibhat added the Medium A valid Medium severity issue label Jul 7, 2023
@sherlock-admin sherlock-admin added Escalation Resolved This issue's escalations have been approved/rejected and removed Escalated This issue contains a pending escalation labels Jul 7, 2023
@sherlock-admin2 sherlock-admin2 added the Has Duplicates A valid issue with 1+ other issues describing the same vulnerability label Jul 17, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Escalation Resolved This issue's escalations have been approved/rejected Has Duplicates A valid issue with 1+ other issues describing the same vulnerability Medium A valid Medium severity issue Reward A payout will be made for this issue
Projects
None yet
Development

No branches or pull requests

5 participants