Extreme edge case error results in user paying more fees than usual when user decides to mint little shTokens with low decimals tokens

[c4-bot-4]

Lines of code

https://github.com/code-423n4/2023-11-shellprotocol/blob/485de7383cdf88284ee6bcf2926fb7c19e9fb257/src/ocean/Ocean.sol#L1068-L1109

Vulnerability details

Proof of Concept

The wrapping and unwrapping process is mostly foolproof even with low/high decimal tokens.

There is an edge case if a person intends to mint ~1e12 shTokens with a low decimal token

Let's say that token ABC has 2 decimals and is worth 20000 USDC (1e2 ABC tokens == 20000)

User wants to mint 1e15 shABC tokens

• `convertDecimals(18,2,1e15)
• convertedAmount = 0, truncatedAmount = 1000000000000000
• Since truncatedAmount > 0, Line 1084 is executed
• transferAmount = 1
• `converDecimals(2,18,1)
• normalizedTransferAmount = 10000000000000000, normalizedTruncatedAmount = 0
• dust = normalizedTransferAmount - 1e15 = 9e15
• transferAmount = 1, dust = 9e15

In order to mint 1e15 shTokens, user has to pay 1 wei token to get 1e15 tokens. 9e15 shTokens is paid to the the protocol. By right, 1 wei token should get the user 1e16 shTokens instead of 1e15 shTokens

Code Reference:

    function _determineTransferAmount(
        uint256 amount,
        uint8 decimals
    )
        private
        pure
        returns (uint256 transferAmount, uint256 dust)
    {
        // if (decimals < 18), then converting 18-decimal amount to decimals
        // transferAmount will likely result in amount being truncated. This
        // case is most likely to occur when a user is wrapping a delta as the
        // final interaction in a transaction.
        uint256 truncated;

        (transferAmount, truncated) = _convertDecimals(NORMALIZED_DECIMALS, decimals, amount);

        if (truncated > 0) {
            // Here, FLOORish(x) is not to the nearest integer less than `x`,
            // but rather to the nearest value with `decimals` precision less
            // than `x`. Likewise with CEILish(x).
            // When truncating, transferAmount is FLOORish(amount), but to
            // fully cover a potential delta, we need to transfer CEILish(amount)
            // if truncated == 0, FLOORish(amount) == CEILish(amount)
            // When truncated > 0, FLOORish(amount) + 1 == CEILish(AMOUNT)
            transferAmount += 1;
            // Now that we are transferring enough to cover the delta, we
            // need to determine how much of the token the user is actually
            // wrapping, in terms of 18-decimals.
            (uint256 normalizedTransferAmount, uint256 normalizedTruncatedAmount) =
                _convertDecimals(decimals, NORMALIZED_DECIMALS, transferAmount);
            // If we truncated earlier, converting the other direction is adding
            // precision, which cannot truncate.
            assert(normalizedTruncatedAmount == 0);
            assert(normalizedTransferAmount > amount);
            dust = normalizedTransferAmount - amount;
        } else {
            // if truncated == 0, then we don't need to do anything fancy to
            // determine transferAmount, the result _convertDecimals() returns
            // is correct.
            dust = 0;
        }
    }

Impact

User pays more fees than normal when wrapping tokens with low decimals and minting low amounts of shTokens.

Tools Used

VSCode

Recommended Mitigation Steps

Recommend not allowing minting such a low amount of shTokens and also possibly disallowing extremely low decimal tokens.

Assessed type

Error

[c4-pre-sort]

raymondfam marked the issue as sufficient quality report

[c4-pre-sort]

raymondfam marked the issue as duplicate of #169

[c4-judge]

0xA5DF changed the severity to QA (Quality Assurance)

[c4-judge]

0xA5DF marked the issue as grade-c

[0xA5DF]

Moved to QA report

[c4-judge]

0xA5DF marked the issue as grade-a