Truncating decimals in _convertDecimals() causes ERC20 token value loss

[c4-bot-3]

Lines of code

https://github.com/code-423n4/2023-11-shellprotocol/blob/485de7383cdf88284ee6bcf2926fb7c19e9fb257/src/adapters/OceanAdapter.sol#L138-L159

Vulnerability details

Impact

An erroneous implementation of decimals conversion inside OceanAdapter.sol cuts less significant digits of ERC20 tokens that use more than 18 digits. This leads to an unjustified loss of value during every type of ComputeType action.

Furthermore, we didn't find any restriction on token decimals, and we assume that every ERC20-compliant contract could be implemented and used inside Ocean. In fact, OceanAdapter._convertDecimals() is written to manage both cases: decimalsFrom < decimalsTo and decimalsFrom > decimalsTo. We want also underlying that tests of this function are not exhaustive, because they only check usdc->usdt and usdt->usdc swap (both use 6 decimals).

Finally, referring to Can an ERC-20 have more than 18 decimals?, we should assume that an ERC20 implementation could use at most 77 decimals. Even if this implementation may be problematic, it is possible. In a more likely future implementation of some ERC20 token, could be possible to use an ERC20 with 32 decimals, for example.

Proof of Concept

This issue is present both in Curve2PoolAdapter.sol and CurveTricryptoAdapter.sol. We are going to continue PoC with the former, but similar considerations can be applied on the latter.

In Curve2PoolAdapter.sol#L142-L184 there is primitiveOutputAmount() function. This function is called from Ocean contract in _executeInteraction() when it receives Curve2PoolAdapter.sol address as externalContract parameter. This is primitiveOutputAmount() function:

142      function primitiveOutputAmount(
143          uint256 inputToken,
144          uint256 outputToken,
145          uint256 inputAmount,
146          bytes32 minimumOutputAmount
147      )
148          internal
149          override
150          returns (uint256 outputAmount)
151      {
152          uint256 rawInputAmount = _convertDecimals(NORMALIZED_DECIMALS, decimals[inputToken], inputAmount);
153  
154          ComputeType action = _determineComputeType(inputToken, outputToken);
155  
156          uint256 rawOutputAmount;
157  
158          // avoid multiple SLOADS
159          int128 indexOfInputAmount = indexOf[inputToken];
160          int128 indexOfOutputAmount = indexOf[outputToken];
161  
162          if (action == ComputeType.Swap) {
163              rawOutputAmount =
164                  ICurve2Pool(primitive).exchange(indexOfInputAmount, indexOfOutputAmount, rawInputAmount, 0);
165          } else if (action == ComputeType.Deposit) {
166              uint256[2] memory inputAmounts;
167              inputAmounts[uint256(int256(indexOfInputAmount))] = rawInputAmount;
168              rawOutputAmount = ICurve2Pool(primitive).add_liquidity(inputAmounts, 0);
169          } else {
170              rawOutputAmount = ICurve2Pool(primitive).remove_liquidity_one_coin(rawInputAmount, indexOfOutputAmount, 0);
171          }
172  
173          outputAmount = _convertDecimals(decimals[outputToken], NORMALIZED_DECIMALS, rawOutputAmount);
174  
175          if (uint256(minimumOutputAmount) > outputAmount) revert SLIPPAGE_LIMIT_EXCEEDED();
176  
177          if (action == ComputeType.Swap) {
178              emit Swap(inputToken, inputAmount, outputAmount, minimumOutputAmount, primitive, true);
179          } else if (action == ComputeType.Deposit) {
180              emit Deposit(inputToken, inputAmount, outputAmount, minimumOutputAmount, primitive, true);
181          } else {
182              emit Withdraw(outputToken, inputAmount, outputAmount, minimumOutputAmount, primitive, true);
183          }
184      }

Assuming we are performing a swap operation. Furthermore, let's assume that inputToken and outputToken are tokens ERC20 compliant with 32 decimals. At line 173, after swapping, contract call _convertDecimals() on this token, passing decimals[outputToken] that we assume is 32 and NORMALIZED_DECIMALS, i.e. 18.

Now, let's analyze _convertDecimals() function, inside OceanAdapter.sol:

138      function _convertDecimals(
139          uint8 decimalsFrom,
140          uint8 decimalsTo,
141          uint256 amountToConvert
142      )
143          internal
144          pure
145          returns (uint256 convertedAmount)
146      {
147          if (decimalsFrom == decimalsTo) {
148              // no shift
149              convertedAmount = amountToConvert;
150          } else if (decimalsFrom < decimalsTo) {
151              // Decimal shift left (add precision)
152              uint256 shift = 10 ** (uint256(decimalsTo - decimalsFrom));
153              convertedAmount = amountToConvert * shift;
154          } else {
155              // Decimal shift right (remove precision) -> truncation
156              uint256 shift = 10 ** (uint256(decimalsFrom - decimalsTo));
157              convertedAmount = amountToConvert / shift;
158          }
159      }

In our case, decimalsFrom = 32 and decimalsTo = 18, so function will execute this portion of code:

154          } else {
155              // Decimal shift right (remove precision) -> truncation
156              uint256 shift = 10 ** (uint256(decimalsFrom - decimalsTo));
157              convertedAmount = amountToConvert / shift;
158          }

This will cut digits that are less significant than 10^15. So after swapping, the user loses several decimal amounts without any reason. Even if it could be a small amount (if we are thinking about a token with 32 decimals, it will be comparable with a gwei), enough amount of interactions inside _doMultipleInteraction could lead to a big loss. For this reason, we think this issue should be high.

Tools Used

Visual inspection

Recommended Mitigation Steps

It could be better that before normalizing, the amount lost thanks to conversion is sent to the user or accumulated in a pool.

Assessed type

Decimal

[c4-pre-sort]

raymondfam marked the issue as insufficient quality report

[c4-pre-sort]

raymondfam marked the issue as duplicate of #135

[c4-judge]

0xA5DF marked the issue as unsatisfactory:
Invalid

[niser93]

Hi @0xA5DF! Thank you for your judgment.
I'd like to express my point of view more deeply because I think this is a valid problem.
The aim of the Shell Protocol and the Ocean is to create common primitives and an environment where these primitives can be composed. In this scenario, every kind of ERC20 should be taken into account.
The function OceanAdapter.convertDecimal() truncates less significant decimals without tracking them. So, it doesn't care about dust for every ERC20 token. For every token that works with more than 18 decimals, there will be a small deviation between the primitiveOutputAmount() output and the real amount of tokens swapped on the Ocean.
This means that in case of multiple interactions, balanceDeltas in Ocean can deviate from the correct value. In fact, Ocean aims to help to create also big composition of primitives. In the case of a big implementation of multiple swap interactions, the final deviation can be very consistent.
I couldn't prove it using code because developers only implemented tokens with 6 decimals. However, if you think it will be necessary, I'll try to do it (in just 48 hours can be very hard). I would appreciate also an opinion of the Shell Team about that, in order to understand if it is not a problem from their point of view, and why.

I would also remark that my issue seems not a duplicate of #135. It suggests to remove dust, while i'm suggesting to manage dust also inside OceanAdapter contract.

Thanks in advance for your work

[0xA5DF]

Hey @niser93
I see that this finding is based on using a token that has 32 decimals.
Can you point out such a token that's commonly used on Arbitrum?
If not then I don't think this would qualify as more than a low.

[niser93]

Thank you for the answer. My POC is based on a token that has 32 decimals just to stress the importance of deviation. The problem is valid if the token uses more than 18 decimals, for example, 20 decimals. An example can be YAMv2, with 24 decimals, which seems not used anymore. Other example is NEAR token on Ethereum, with 24 decimals.

According to OpenZeppelin ERC20 documentation:

 * The default value of {decimals} is 18. To change this, you should override
 * this function so it returns a different value.
 [...]

* Tokens usually opt for a value of 18, imitating the relationship between
* Ether and Wei. This is the default value returned by this function, unless
* it's overridden.

For this reason, all tokens commonly used have 18 decimals. Furthermore, many topics suggest developers use 18 decimals. However, Ocean aims to manage ERC20 with any number of decimals. Due to the purpose of the protocol, it should be invariant with respect to token decimals. In contest invariant section

The Ocean should conform to all standards that its code claims to (ERC-1155, ERC-165
[...]
The Ocean does not support rebasing tokens, fee on transfer tokens

There were proposals to standardize 18-decimal tokens (see this). However, to date ERC20 token with more than 18 decimals belongs to standard, and without different specifications on the contest page, I think this issue could be considered as a deviation from the expected behavior.

[0xA5DF]

Got it, taking another look at the issue it seems that even in those cases the amount would be insignificant.
Let's assume that each unit of YAMv2 (1e24 wei) is worth 1000 USD, and the user is swapping 1 USD worth of YAMv2 - which would be 1e21 wei.
The truncation would indeed be by 6 decimals, but that would be worth 1e-15 USD, which I think we could all agree is an insignificant amount.

[niser93]

Yes, it will be insignificant amount per interactions. But Ocean gives the possibility to perform hundreds, or thousands of interactions using doMultipleInteraction. Furthermore, there is the possibility to create primitives made by multiple interactions that can be used to compose other primitives, making the problem exponential. Finally, the deviation can only increase so this problem should become significant over time. An user that usually will swap ERC20 token (with more than 18 decimals) would incur in consistent losses.
The Ocean aims to compose multiple primitive tracking the balance in memory, in order to save gas. So, we can also think that it could be used to make billions of micro-transaction (or micro-interaction), because it would be less expensive in term of gas cost.

[0xA5DF]

But Ocean gives the possibility to perform hundreds, or thousands of interactions

Even with a billion interactions, 1e-15 times 1 billion would be 1e-6 USD. Does that seem like a significant amount to you?

[niser93]

I'm not able to estimate how many interactions will be done. I agree that they should be at least 10e20 to have a significant losses. I would like to point you attention on the fact that convertDecimal() is inside OceanAdapter, and this abstract contract will be inherited by every curve adapter. So many pools and other kind of contracts could be built on top of this primitive. Curve2PoolAdapter and CurveTricryptoAdapter are just a basic bricks on top of that it can built many applications. So it could be very hard estimate how many interactions can be affected.

[0xA5DF]

So it could be very hard estimate how many interactions can be affected.

Even with a trillion transactions it would still not be significant, and I think it's safe to say there aren't going to be more than a trillion txs in the foreseeable future

[niser93]

Ok! I absolutely accept your judgement. Thank you for your time and your clarifications

[0xA5DF]

No problem