Bug in deposit if the asset decimals are less than 18

[andreivladbrg]

About

In the current version of the deposit function it requires a 18 decimal amount param.

This causes a bug when the asset has less than 18 decimals due to a problem here:

flow/src/SablierFlow.sol

Line 413 in aac36d5

return (amount / (10 ** normalizingFactor)).toUint128();

Imagine this scenario for a 6 decimals asset:

• a stream is created
• a deposit is made with an amount of assets (let's say 1_000000_500000000000)
  • notice the "5" digit right after the 6th decimals
• _streams[streamId].balance will be updated to 1_000000_500000000000
  • the asset.balanceOf(flow) = 1_000000
• a second deposit it's made with the same amount
• _streams[streamId].balance will be updated to 2 * 1_000000_500000000000 = 2_000001_000000000000
  • notice the shifted digit to the left (due to 5 + 5)
  • the asset.balanceOf(flow) = 2_000000

So, now we will have a stream balance, normalized (2_000001), greater than the actual asset balance (2_000000) assigned to the unique stream, which as you can see, it can lead to a leak of funds.

Note: For extract operations (refund and withdraw), I couldn’t find a problem because the amount is subtracted and not added, so we will not have problems with digits shifting to the left.

Test on remix

    function bugTest()
        public
        pure
        returns (uint128 afterSecondDepositStreamBal, uint128 afterSecondDepositAssetBalanceOf, uint128 streamBalAfterSecondDepositNormalized)
    {
        // There is a first deposit made with 1_000000_500000000000, so:

        uint128 currentAssetBalanceOf = 1_000000;
        uint128 currentStreamBal = 1_000000_500000000000;

        // Deposit the same amount
        uint128 depositAmount = currentStreamBal;

        afterSecondDepositStreamBal = currentStreamBal + depositAmount; // result: 2_000001_000000000000
        afterSecondDepositAssetBalanceOf = currentAssetBalanceOf + _calculateTransferAmount(depositAmount, 6); // result: 2_000000
        streamBalAfterSecondDepositNormalized = _calculateTransferAmount(afterSecondDepositStreamBal, 6); // 2_000001
    }

    function _calculateTransferAmount(uint128 amount, uint8 assetDecimals) internal pure returns (uint128) {
        // Return the original amount if asset's decimals are already 18.
        if (assetDecimals == 18) {
            return amount;
        }

        if (assetDecimals > 18) {
            uint8 normalizingFactor = assetDecimals - 18;
            return (amount * (10 ** normalizingFactor)).toUint128();
        } else {
            uint8 normalizingFactor = 18 - assetDecimals;
            return (amount / (10 ** normalizingFactor)).toUint128();
        }
    }

How to fix this

The fix consists of changing the amount parameter to match the asset decimals. However, the internal storage can still be in an 18-decimal format, so we will need to add a new function _calculateDepositAmount.

For assets with decimals greater than 18, I believe we will need to restrict the last decimals to zeros (the difference between decimals and 18). For example, the transfer amount divided by $10^{18}$ must be equal to $10^{normalizingFactor}$ so that assets won’t be stuck in the contract. Yes, it is a limitation, but it wouldn’t lead to loss of funds from other streams. Also, there are almost no assets with more than 18 decimals.

    function _calculateDepositAmount(uint128 transferAmount, uint8 assetDecimals) internal pure returns (uint128) {
        // Return the original amount if asset's decimals are already 18.
        if (assetDecimals == 18) {
            return transferAmount;
        }

        if (assetDecimals > 18) {
            uint8 normalizingFactor = assetDecimals - 18;
            uint256 factor = 10 ** normalizingFactor;

            if (uint256(transferAmount) / 1e18 != factor) {
                revert InvalidDepositAmount();
            }
        
            return (transferAmount / factor).toUint128();
        } else {
            uint8 normalizingFactor = 18 - assetDecimals;
            return (transferAmount * (10 ** normalizingFactor)).toUint128();
        }
    }

---

@smol-ninja Please lmk if I have missed something in my proposed fix, or if you have a better idea on how address this bug. Otherwise I will open a PR with this fix.

[smol-ninja]

Great sleuthing @andreivladbrg. I am glad you found it.

While reading your report, I stumbled upon a question for which I have created a discussion: #150 and would love like to hear your thoughts on it.

If we agree to keep the 18-decimals format, then I cannot think of a better solution than what you have already proposed. We should also add two more assets, 0 decimals and 30 decimals, to the list of assets for invariant testing (related issue: #137).

[smol-ninja]

I have another idea.

What if we allow deposits in native decimals instead of normalised decimals? In your solution, you will now make two conversions:

1. input amount to transfer amount using _calculateTransferAmount
2. transfer amount to normalised using _calculateDepositAmount to mitigate calculation errors

If we allow deposit with native decimals, then we only have to handle one conversion i.e. _calculateDepositAmount (or _calculateNormalizedAmount I would say). To maintain consistency, we can also refactor refund.

• The public APIs will take amounts in native decimals.
• Internally, contract use 18 decimals.
• withdraw takes time so there is no problem in that.
• No change in adjustRatePerSecond as well

Do you find any problem with this approach?

[andreivladbrg]

What if we allow deposits in native decimals instead of normalised decimals? In your solution, you will now make two conversions:

1. input amount to transfer amount using _calculateTransferAmount
2. transfer amount to normalised using _calculateDepositAmount to mitigate calculation errors

Wait, I don't think I understand. Are you suggesting to make the _streams[id]balance as the native asset decimals?

In my proposal, if I was not clear. I am saying that in the deposit function we will have the amount passed as the native assets decimals, and then we will use only _calculateDepositAmount

---

Also, I am afraid we can’t escape the need for a second conversion because the refundable/withdrawable amounts are in an 18-decimal format, as we need to have the rps in an 18-decimal format. In the _extractFromStream function, we will need two amounts: transfer amount and the amount to subtract from the balance

[smol-ninja]

In my proposal, if I was not clear. I am saying that in the deposit function we will have the amount passed as the native assets decimals, and then we will use only _calculateDepositAmount

Oh that was not clear to me from the OP. Then we are on the same page.

Can you explain did you add revert InvalidDepositAmount statement?

we can’t escape the need for a second conversion because the refundable/withdrawable amounts

Thats true, we will need both. But not necessary in the same function.

What do you think about using the same design in refund function, i.e. amount is in native asset?

[andreivladbrg]

Can you explain did you add revert InvalidDepositAmount statement

It is in the OP, but I will paste it here again:

For assets with decimals greater than 18, I believe we will need to restrict the last decimals to zeros (the difference between decimals and 18). For example, the transfer amount divided by $10^{18}$ must be equal to $10^{normalizingFactor}$ so that assets won’t be stuck in the contract. Yes, it is a limitation, but it wouldn’t lead to loss of funds from other streams. Also, there are almost no assets with more than 18 decimals.

---

But not necessary in the same function

Ofc not in the same function. There will be a problem if you use both in the same function. Either one way, either the other way.

What do you think about using the same design in refund function, i.e. amount is in native asset

we could do that as it would be technically possible. but there would an inconsistency between the 2 extract operations :

1. From 18-decimal to asset decimals

• In withdraw function you will calculate the withdrawable amount as 18 decimal format, then you will convert it to the transfer amount

2. From asset decimals to 18-decimal

• In refund function, you will convert the amount to 18 decimal format, then you will transfer the amount passed and subtracted the calculated amount

in the same time, you can say there will an inconsistency between amount params:

1. One denoted in asset decimals
2. One denoted in 18 decimals

Not sure, yet, what is the best approach.
Let's sleep on the idea, and see how the change looks in the code.

---

Should I proceed with creating a PR with proposed fix?

[smol-ninja]

I believe we will need to restrict the last decimals to zeros (the difference between decimals and 18)

I see. I am not in favour of reverting the deposit tx. Instead of reverting, lets just transfer the correct amount in transferFrom function.

• User calls function with 10.000..(20 decimals)..005.
• We normalize it to 10.00..(18 decimals)..00.
• We transfer 10.000..(20 decimals)..000.

A reverting tx will cost a significant amount of gas. Even the UI can make such mistakes.

Should I proceed with creating a PR with proposed fix?

Yes please. Thank you.