If primitive calls back into Ocean before output token burn, balance manipulation possible.

[c4-bot-8]

Lines of code

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

Vulnerability details

Impact

There is a risk of reentrancy if a primitive callback into the Ocean before the input token burn occurs. _computeOutputAmount burns output tokens after the call: Link to the function

    function _computeOutputAmount(
        address primitive,
        uint256 inputToken,
        uint256 outputToken,
        uint256 inputAmount,
        address userAddress,
        bytes32 metadata
    )
        internal
        returns (uint256 outputAmount)
    {

    // Transfer input tokens 
    _increaseBalanceOfPrimitive(primitive, inputToken, inputAmount);

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

 // Callback happens here

    _decreaseBalanceOfPrimitive(primitive, outputToken, outputAmount);

}

If the primitive callback into the Ocean before output tokens were burned, it could manipulate its balance.

This could allow adversarial withdrawals or balance manipulation. If $10M in assets were drained, it may cause insolvency.

The attack would involve:

• Call computeOutputAmount
  • Primitive callback occurs
  • Callback manipulates balance before burn

Proof of Concept

The _computeOutputAmount updates primitive's balance in two steps: Link to Steps

        _increaseBalanceOfPrimitive(primitive, inputToken, inputAmount);

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

        _decreaseBalanceOfPrimitive(primitive, outputToken, outputAmount);

        emit ComputeOutputAmount(primitive, inputToken, outputToken, inputAmount, outputAmount, userAddress);
    }

If primitive recursively calls back into _computeOutputAmount after input tokens were minted but before outputs were burned, it could artificially increase its balance.

A malicious primitive could drain user funds or protocol treasury. For example, $10 million lost if exploited. An attack could - Call _computeOutputAmount - Recursive call back to _computeOutputAmount

For example:

1. User swaps 10 INPUT -> 10 OUTPUT 
2. 10 INPUT minted to primitive
3. Primitive call back, swaps 1 INPUT -> 100 OUTPUT  
4. Withdraws 100 OUTPUT while only burning 10

Could drains funds.

Recommended Mitigation Steps

Use reentrancy guard:

Assessed type

Context

[c4-pre-sort]

raymondfam marked the issue as insufficient quality report

[c4-pre-sort]

raymondfam marked the issue as duplicate of #105

[c4-judge]

0xA5DF marked the issue as unsatisfactory: Invalid