dacian - Precision loss to amountToBuyLeftUSD, amountToSellUnits in USSDRebalancer.BuyUSSDSellCollateral()

[sherlock-admin]

dacian

high

Precision loss to amountToBuyLeftUSD, amountToSellUnits in USSDRebalancer.BuyUSSDSellCollateral()

Summary

Precision loss to amountToBuyLeftUSD & amountToSellUnits in USSDRebalancer.BuyUSSDSellCollateral().

Vulnerability Detail

First examine USSDRebalancer.rebalance() L97:

BuyUSSDSellCollateral((USSDamount - DAIamount / 1e12)/2);

In this function call to BuyUSSDSellCollateral(), a final division by 2 is performed.

Next examine USSDRebalancer.BuyUSSDSellCollateral():

function BuyUSSDSellCollateral(uint256 amountToBuy) internal {
  CollateralInfo[] memory collateral = IUSSD(USSD).collateralList();
  //uint amountToBuyLeftUSD = amountToBuy * 1e12 * 1e6 / getOwnValuation();
  uint amountToBuyLeftUSD = amountToBuy * 1e12;

Here amountToBuy which was previously divided by 2 is now multiplied again and the result stored in amountToBuyLeftUSD. This results in precision loss due to Division Before Multiplication. The same thing also occurs at L148-149:

if (DAItosell > amountToBuy * 1e12 * 99 / 100) {
  DAItosell = amountToBuy * 1e12 * 99 / 100;

Next examine L121:

uint256 amountToSellUnits = IERC20Upgradeable(collateral[i].token).balanceOf(USSD) * ((amountToBuyLeftUSD * 1e18 / collateralval) / 1e18) / 1e18;

Here amountToBuyLeftUSD that has already been divided, is now multiplied then divided again, resulting in more precision loss.

Impact

Incorrect amounts will be bought & sold.

Code Snippet

I have created a stand-alone test case to show the first precision loss using Foundry. Contract functions:

function ussdErrorAmountToBuy(uint ussdAmount, uint daiAmount) 
    public view returns (uint256) {
    console.log("ussdErrorAmountToBuy");
    console.log("ussdAmount : ", ussdAmount);
    console.log("daiAmount  : ", daiAmount);

    // @audit /2 * 1e12 division before multiplication
    // causes precision loss
    uint result = (ussdAmount - daiAmount / 1e12)/2 * 1e12;

    console.log("result     : ", result);
    return result;
}

function ussdCorrectAmountToBuy(uint ussdAmount, uint daiAmount) public view returns (uint256) {
    console.log("ussdCorrectRebalance");
    console.log("ussdAmount : ", ussdAmount);
    console.log("daiAmount  : ", daiAmount);

    // @audit /2 * 1e12 can be rewritten as * 1e12 / 2,
    // removes division before multiplication, solving precision loss
    uint result = (ussdAmount - daiAmount / 1e12) * 1e12 / 2;

    console.log("result     : ", result);
    return result;
}

And the test function:

function testUssdAmountToBuy() public {
    console.log("case #1: everything works out, no precision loss");      
    uint uusdAmount = 23254_123456;             // 6 decimals 
    uint daiAmount  = 15379_123456789123456789; // 18 decimals 

    uint errorOutput   = vulnContract.ussdErrorAmountToBuy(uusdAmount, daiAmount);
    uint correctOutput = vulnContract.ussdCorrectAmountToBuy(uusdAmount, daiAmount);

    assertEq(errorOutput, correctOutput);

    console.log("case #2: slightly modify input, now precision loss visible");    
    uusdAmount = 23254_123451;

    errorOutput   = vulnContract.ussdErrorAmountToBuy(uusdAmount, daiAmount);
    correctOutput = vulnContract.ussdCorrectAmountToBuy(uusdAmount, daiAmount);

    assert(errorOutput != correctOutput);
}

Running gives the output:

case #1: everything works out, no precision loss
ussdErrorAmountToBuy
ussdAmount :  23254123456
daiAmount  :  15379123456789123456789
result     :  3937500000000000000000
ussdCorrectRebalance
ussdAmount :  23254123456
daiAmount  :  15379123456789123456789
result     :  3937500000000000000000
case #2: slightly modify input, now precision loss visible
ussdErrorAmountToBuy
ussdAmount :  23254123451
daiAmount  :  15379123456789123456789
result     :  3937499997000000000000 // @audit note precision loss here
ussdCorrectRebalance
ussdAmount :  23254123451
daiAmount  :  15379123456789123456789
result     :  3937499997500000000000 // @audit actual value without precision loss

Tool used

Manual Review

Recommendation

/2 * 1e12 can be rewritten as * 1e12 / 2, removing division before multiplication & solving precision loss.

((amountToBuyLeftUSD * 1e18 / collateralval) / 1e18) can be rewritten as (amountToBuyLeftUSD / collateralval) as multiplying and dividing by 1e18 cancels out, this removes the multiplication after division and solves the precision loss.

Duplicate of #187

[devdacian]

Escalate for 10 USDC

This issue has been marked as low & non-reward due to: "Impact of this seems to be of the order 0.000001 USD. Considering this issue a low".

Please consider that a total loss of precision can occur, where the original version results in amountToBuyLeftUSD = 0 but the fixed version results in amountToBuyLeftUSD = 500000000000, which is a complete loss of value.

I have written a long article where I prove this using an invariant fuzz test; I haven't reproduced it all here as it is very long and contains much code. Please consider my findings in this article before marking this as low/excluded, since there are at least 2 inputs sets where value is completely lost as the original version erroneously calculates to 0:

// case 1
originalOutput   :  0
simplifiedOutput :  500000000000
maxPrecisionLoss :  500000000000
// inputs
mplUssdAmount    :  1000001
mplDaiAmount     :  1000000000000000002

// case 2
originalOutput   :  0
simplifiedOutput :  500000000000
maxPrecisionLoss :  500000000000
// inputs
mplUssdAmount    :  1000000000000000
mplDaiAmount     :  999999999999999999999999999

When amountToBuyLeftUSD = 0 in BuyUSSDSellCollateral() due to this precision loss, the remaining code will fail to sell any collateral since for example amountToSellUnits is calculated by mutliplying amountToBuyLeftUSD which is 0 instead of 500000000000 due to the precision loss.

If my escalation is successful, I kindly request that my submission is marked as unique to the other duplicates of #187 since the others don't identify the total precision loss and the impact it has upon the non-selling of any collateral. My linked invariant fuzz test proves precision loss which results in total loss of value & provides 2 input sets where this is the case, providing maximum value to the Sponsor and to future readers of the audit report.

If my escalation is successful but the judges decide that it is not unique from #187, then at the very least my submission should not be a duplicate of 659, 968 & 470 as those issues address precision loss in a different part of the code than the one covered in this issue.

[sherlock-admin]

Escalate for 10 USDC

This issue has been marked as low & non-reward due to: "Impact of this seems to be of the order 0.000001 USD. Considering this issue a low".

Please consider that a total loss of precision can occur, where the original version results in amountToBuyLeftUSD = 0 but the fixed version results in amountToBuyLeftUSD = 500000000000, which is a complete loss of value.

I have written a long article where I prove this using an invariant fuzz test; I haven't reproduced it all here as it is very long and contains much code. Please consider my findings in this article before marking this as low/excluded, since there are at least 2 inputs sets where value is completely lost as the original version erroneously calculates to 0:

// case 1
originalOutput   :  0
simplifiedOutput :  500000000000
maxPrecisionLoss :  500000000000
// inputs
mplUssdAmount    :  1000001
mplDaiAmount     :  1000000000000000002

// case 2
originalOutput   :  0
simplifiedOutput :  500000000000
maxPrecisionLoss :  500000000000
// inputs
mplUssdAmount    :  1000000000000000
mplDaiAmount     :  999999999999999999999999999

When amountToBuyLeftUSD = 0 in BuyUSSDSellCollateral() due to this precision loss, the remaining code will fail to sell any collateral since for example amountToSellUnits is calculated by mutliplying amountToBuyLeftUSD which is 0 instead of 500000000000 due to the precision loss.

If my escalation is successful, I kindly request that my submission is marked as unique to the other duplicates of #187 since the others don't identify the total precision loss and the impact it has upon the non-selling of any collateral. My linked invariant fuzz test proves precision loss which results in total loss of value & provides 2 input sets where this is the case, providing maximum value to the Sponsor and to future readers of the audit report.

If my escalation is successful but the judges decide that it is not unique from #187, then at the very least my submission should not be a duplicate of 659, 968 & 470 as those issues address precision loss in a different part of the code than the one covered in this issue.

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]

Intend to give this finding a unique medium because of the exceptional prove

https://dacian.me/exploiting-precision-loss-via-fuzz-testing

[hrishibhat]

Result: Low Duplicate of #187 The state of the issue remains as low. From the above example it still shows that the impact is dust amount and the since this is happening during rebalancing bringing in a negligible amount lesser can be considered low.

[sherlock-admin]

Escalations have been resolved successfully!

Escalation status:

• devdacian: rejected