A malicious token params can lead to permanent loss of user’s fund

[alsldl]

Severity

High, it can lead to permanent loss of user's fund .

Details

In the specific step, if the formula correct stepPrice * stepRange < 10 ^ token.decimals , it can be lead to loss user’s fund.

In the mint , reserveToBond calculated by ceilDiv(supplyLeft * steps[i].price, multiFactor);

for (uint256 i = getCurrentStep(token, currentSupply); i < steps.length; ++i) {
    supplyLeft = steps[i].rangeTo - currentSupply;

    if (supplyLeft < tokensLeft) {
        if(supplyLeft == 0) continue;

        // ensure reserve is calculated with ceiling
        reserveToBond += Math.ceilDiv(supplyLeft * steps[i].price, multiFactor);
        currentSupply += supplyLeft;
        tokensLeft -= supplyLeft;
    } else {
        // ensure reserve is calculated with ceiling
        reserveToBond += Math.ceilDiv(tokensLeft * steps[i].price, multiFactor);
        tokensLeft = 0;
        break;
    }
}

Even if the expression stepPrice * stepRange < 10 ^ token.decimals is correct, it will be rounded up and treated as 1 token.

The user have to pay reserveToBond to buy token. And reserveToBond larger than 0.

However, in the burn , getRefundForTokens

while (i >= 0 && tokensLeft > 0) {
    uint256 supplyLeft = i == 0 ? currentSupply : currentSupply - steps[i - 1].rangeTo;

    uint256 tokensToProcess = tokensLeft < supplyLeft ? tokensLeft : supplyLeft;
    reserveFromBond += ((tokensToProcess * steps[i].price) / multiFactor);

    tokensLeft -= tokensToProcess;
    currentSupply -= tokensToProcess;

    if (i > 0) --i;
}

Here, we're calculating by rounding down, not rounding up.

reserveFromBond += ((tokensToProcess * steps[i].price) / multiFactor);

As result, reserveFromBond added zero by rounding down.

However tokensLeft and currentSupply decreased by tokensToProcess.

User will refund by reserveFromBond , but it is zero by rounding down.

As result, the user loss their funds.

Recommend

It is not possible to fix getRefundForTokens. because non-decrementing may result in additional withdrawals.

It is possible to add syntax to validateBondParam to check for this, but it would require iterating through the loop as many times as there are steps.
So, In gas-cost optimization perspective, it makes sense to modify setBond, which already has the logic to advance the loop.

function _setBond(address token, BondParams calldata bp) private {
    ...
    for (uint256 i = 0; i < bp.stepRanges.length; ++i) {
        ...
        if (bp.stepRanges[i]*bp.stepPrices[i] >= 10 ** token.decimals())
            revert("It can be used fraudulently");
        ...
    }
}

[weed0607]

I think rounding up and down when buying and selling is dev intention.
Users should consider such conditions, in my opinion.
But I am not judge anyway, our sponsor will do the job.

[alsldl]

I think rounding up and down when buying and selling is dev intention. Users should consider such conditions, in my opinion. But I am not judge anyway, our sponsor will do the job.

I agree that they intend the formula.
However, if there are no condition i refer above, the user have to check every token step and parameter's when they buy the tokens.
If the users doesn't, the user loses fund and can't recover it.

[Paschal695]

I think with right slippage,user loss will be reduced to the barest minimum,as long as we’re approximating there sure will be negligible loss

[alsldl]

I think with right slippage,user loss will be reduced to the barest minimum,as long as we’re approximating there sure will be negligible loss

I think you've misunderstood.

This is not a problem caused by price fluctuations.
In the above-mentioned situation, no matter how the user sets the slippage, they will receive zero assets back.

No matter how the user sets the slippage, if they don't specify a value of zero to be returned, a revert will occur and the function will fail.

[sydneyitguy]

Yes, I am aware of the issue, and I have written test cases for those edge cases.
REF: https://github.com/Steemhunt/mint.club-v2-contract/blob/main/test/Bond.test.js#L756

Because it is very rare for ERC20 tokens to have fewer than 6 decimal places, I thought it wouldn't be an issue to lose around 1 unit of tokens, which is normally just like 1 wei.

BTW, your suggestion:

if (bp.stepRanges[i]*bp.stepPrices[i] >= 10 ** token.decimals()) revert("It can be used fraudulently");

doesn't seem to be correct because bp.stepRanges[i]*bp.stepPrices[i] is almost always larger than 10 ** token.decimals()
REF: https://github.com/Steemhunt/mint.club-v2-contract/blob/main/test/Bond.test.js#L27

[alsldl]

You are correct that it can be ignored by 1 wei.

However, I wanted to point out that users could split the steps to exploit this.

Consider below situation.

uint256 multiFactor = 10**t.decimals();
The value of multiFactor is 1e18.

stepRanges = [ 100 wei, 100 wei, 100 wei, ...]
stepPrices = [ 0.009 ether, 0.0099 ether, 0.00999 ether, ...]

Regardless of how many tokens a user is issued, there is no way to get back what they paid.

[sydneyitguy]

Your parameter will be treated as invalid because it must increase at every step (ref: https://github.com/Steemhunt/mint.club-v2-contract/blob/main/contracts/MCV2_Bond.sol#L200)

Also, for the first section of your example, reserveOnBond will be calculated as 100 * 0.009 ether = 0.9 ether, which is equivalent to 10^17 wei. Why is it mentioned that "there is no way to get what they paid"?

Regarding the statement

Regardless of how many tokens a user is issued, there is no way to get back what they paid

could you provide an example to help me understand better?

[alsldl]

Sorry, my example is wrong.

Let me try again.
Please correct me if I'm thinking of this incorrectly.

uint256 multiFactor = 10**t.decimals();
The value of multiFactor is 1e18.

stepRanges = [ 100 wei, 200 wei, 300 wei, ...]
stepPrices = [ 0.009 ether, 0.0091 ether, 0.0092 ether, ...]

For ease of calculation, i have excluded royalties.

When user buy 300 wei token.
The user have to pay:

reserveToBond += Math.ceilDiv(supplyLeft * steps[i].price, multiFactor);
=> ceilDiv(100*0.009 ether, 1e18) + ceilDiv(101*0.0091 ether, 1e18) + ceilDiv(102*0.0092 ether, 1e18)
=> ( 0.9 ether / 1 ether ) + ( 0.91 ether / 1 ether ) + ( 0.92 ether / 1 ether )
=> 1 + 1 + 1 ( all cases rounded up)

So, user have to pay 3 wei token.

Next, when user sell 300 wei token.
The user will receive:

reserveFromBond += ((tokensToProcess * steps[i].price) / multiFactor);
=> (100*0.0092 ether / 1e18) + (100*0.0091 ether / 1e18) + (100*0.009 ether / 1e18)
=> ( 0.92 ether / 1 ether ) + ( 0.91 ether / 1 ether ) + ( 0.9 ether / 1 ether )
=> 0 + 0 + 0 ( all cases rounded down)

So, user can receive no token.

This was the problem I was thinking of.
If I'm wrong, please correct me.

[sydneyitguy]

@alsldl
Oh, now I see what your point is. However, those edge cases were intended and unit tested here:
https://github.com/Steemhunt/mint.club-v2-contract/blob/main/test/Bond.test.js#L756

If either stepRanges or stepPrices is an extremely small number, the calculation can create some permanent lock-up of users' funds on the contract by the smallest unit of reserve token (normally 1 wei). This happens because we use Math.ceil for calculating on minting and rounddown on burning.

I thought these differences are negligible because it's only a few weis or a few hundreds of weis, even if it's intentionally designed to be the way you described. And there is no motivation for the creator to intentionally design the curve in that way.

I couldn't find any better solution to solve this issue, so please suggest any if you can think of any.

Your suggestion on the issue:

if (bp.stepRanges[i]*bp.stepPrices[i] >= 10 ** token.decimals()) revert("It can be used fraudulently");

will almost always revert because the parameters normally look like:

stepRanges: [ 1e21, 1e22, 1e23,... 1e25 ],
stepPrices: [ 1e9, 2e9, 3e9, ... 1e10 ]

[alsldl]

I agree for that only can lead to loss of several wei.

My concern was not that this would happen during the normal calculation of the token, but that an attacker could intentionally split the steps to cause this behavior.

If there are multiple of these tokens, users will have to check for them every time they purchase a token.

I also thought that the function might fail if the user received fewer tokens than they expected to receive.

If the user doesn't realize this and sets the amount to be returned, the function will fail to execute.
Also, if the user agrees to accept a smaller return and runs the function, user will lose money due to front-running slippage.

Therefore, users must calculate and fill in how much of their assets may be missing at each run.
I felt this greatly harmed from the user experience.

If you can accept that and move on, I don't think it needs to be fixed.

Additionally, my solution had the inequalities reversed, sorry :D

if (bp.stepRanges[i]*bp.stepPrices[i] < 10 ** token.decimals()) revert("It can be used fraudulently");

Where things can go wrong is when the product of the price and supply at a given step is less than the decimal of the token.

If you don't fix this, the user may lose a small amount of wei due to rounding down, and I think it's a good idea to let users know about a guide to prevent this. :D

[sydneyitguy]

@alsldl I just added your additional validation on createToken params:
468d1d1#diff-a1042ceb3554882b15ff8bd86d8f558acd6101b0ac8c384d2452aa47ed595c6eR200-R203

But it couldn't completely prevent the rounding errors. I added some tests here:
468d1d1#diff-a7cbb63c5634ac553d4585db502488a72de539454029c19064cac94f88902492R817-R871

I just pushed the fix anyway because I think it can still minimize the effect of rounding errors.

Do we have any better solutions? Suggestions are welcome :)

[alsldl]

I was thinking about the example you showed.

I don't think there is a perfect solution, since we can't modify the formula to prevent further withdrawals from the contract.

I think the solution I came up with can only solve a few small cases, but it's not a universal solution.

As you said, you can still add code to minimize rounding errors, and I think you'll be fine without it for gas efficiency!

Since this can happen whether you add code or not, it seems like it would be a best practice to provide a guide that tells the user to set the quantity of tokens to be returned a little less (a few wei), rather than setting the exact number of tokens to be returned so that it doesn't get thrown back due to rounding!

Thanks for taking the time to talk this issue!