code-423n4 / 2023-11-shellprotocol-findings

7 stars 7 forks source link

`doInteraction()` won't mint tokens to user if `interaction.specifiedAmount` less than 10**12 and has certain conditions #287

Closed c4-bot-2 closed 9 months ago

c4-bot-2 commented 9 months ago

Lines of code

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

Vulnerability details

Impact

If the user calls doInteraction() and executes Interaction with specifiedAmount less than 10 ** 12 and a big difference in token decimals, the user won't get any tokens.

Proof of Concept

Core external functions like doInteraction will call the internal _executeInteraction function, which is just a big if... else if... block branching on interaction type. if InteractionType is ComputeOutputAmount or ComputeInputAmount _computeOutputAmount() function will be called, which calls computeOutputAmount

IOceanPrimitive(primitive).computeOutputAmount(inputToken, outputToken, inputAmount, userAddress, metadata);

This computeOutputAmount is protected by onlyOcean modifier, so no one can call it directly.

function computeOutputAmount(...)
        external
        override
        onlyOcean
        returns (uint256 outputAmount)
    {
...
        outputAmount = primitiveOutputAmount(inputToken, outputToken, unwrappedAmount, metadata);
...
    }

primitiveOutputAmount function is virtual and overridden in Curve2PoolAdapter and CurveTricryptoAdapter

Both implementations have this rawInputAmount calculation:

uint256 rawInputAmount = _convertDecimals(NORMALIZED_DECIMALS, decimals[inputToken], inputAmount);

An issue in _convertDecimals function, since NORMALIZED_DECIMALS is a constant that equal 18 and decimals[inputToken] can be UDST that have 6 decimals, after conversion rawInputAmount == 0 if inputAmount is less than 10 ** 12 - 1 PoC

function testOceanAdapterConvertDecimalsTruncatedToZero() public {
        uint8 decimalsFrom = 18;
        uint8 decimalsTo = 6;
        uint256 amountToConvert = 10 ** 12 - 1;

        uint256 outputAmount = _convertDecimals(decimalsFrom, decimalsTo, amountToConvert);
        assertEq(outputAmount, 0);
    }
Running 1 test for src/test/fork/TestOceanAdapter.t.sol:TestOceanAdapter
[PASS] testOceanAdapterConvertDecimalsTruncatedToZero() (gas: 867)
Test result: ok. 1 passed; 0 failed; 0 skipped; finished in 1.79ms

After this logic inside primitiveOutputAmount() of Curve2PoolAdapter or CurveTricryptoAdapter will return zero.

outputAmount in _doInteraction() will be equal 0 and if (outputAmount > 0) statement don’t be executed and user wont get OceanERC1155 tokens

function _doInteraction(
        Interaction calldata interaction,
        address userAddress
    )
        internal
        returns (uint256 inputToken, uint256 inputAmount, uint256 outputToken, uint256 outputAmount)
    {
        ...
            (inputToken, inputAmount, outputToken, outputAmount) = _executeInteraction(
                interaction, interactionType, externalContract, specifiedToken, interaction.specifiedAmount, userAddress
            );
        }
...
        // if _executeInteraction returned a positive value for outputAmount,
        // this amount must be credited to the user's Ocean balance
        if (outputAmount > 0) {
            // since uint, same as (outputAmount != 0)
            _mint(userAddress, outputToken, outputAmount);
        }
    }

Tools Used

foundry

Recommended Mitigation Steps

Add checks for Interaction.specifiedAmount if token decimals is too different.

Assessed type

Under/Overflow

c4-pre-sort commented 9 months ago

raymondfam marked the issue as insufficient quality report

c4-pre-sort commented 9 months ago

raymondfam marked the issue as duplicate of #14

c4-judge commented 9 months ago

0xA5DF marked the issue as unsatisfactory: Invalid