WrapErc721 Interaction Will Almost Never Execute If BalanceDelta Returns Negative

[c4-bot-10]

Lines of code

https://github.com/code-423n4/2023-11-shellprotocol/blob/485de7383cdf88284ee6bcf2926fb7c19e9fb257/src/ocean/Ocean.sol#L510-L524 https://github.com/code-423n4/2023-11-shellprotocol/blob/485de7383cdf88284ee6bcf2926fb7c19e9fb257/src/ocean/Ocean.sol#L634-L645

Vulnerability details

Impact

when the getBalanceDelta function returns a negative value for WrapErc721, and this negative value is used as specifiedAmount during the _executeInteraction function, it will lead to the function always reverting for the _erc721Wrap, as it expects the specifiedAmount always equal to 1.

Proof of Concept

1. It is safe to assume that during _doMultipleInteractions interaction, a user can pass uint256.max as the specifiedAmount when they want to use the total token amount held in the balance delta.

2. Hence, this statement will pass through if (interaction.specifiedAmount == GET_BALANCE_DELTA) { specifiedAmount = balanceDeltas.getBalanceDelta(interactionType, specifiedToken)

3. getBalanceDelta function (in BalanceDelta.sol) will use the interaction type (in this case is WrapErc721) which will return negative delta as shown below:

   function getBalanceDelta(
       BalanceDelta[] memory self,
       InteractionType interaction,
       uint256 tokenId
   )
       internal
       pure
       returns (uint256)
   {
       if (
           interaction == InteractionType.UnwrapErc20 || interaction == InteractionType.UnwrapErc721
               || interaction == InteractionType.UnwrapErc1155 || interaction == InteractionType.UnwrapEther
               || interaction == InteractionType.ComputeOutputAmount
       ) {
           return _getPositiveBalanceDelta(self, tokenId);
       } else {
           // interaction == (Wrap* || ComputeInputAmount)
      @>     return _getNegativeBalanceDelta(self, tokenId);
       }
   }

4. However, when the interaction proceeds to _executeInteraction (for WrapErc721) this statement @> if (specifiedAmount != 1) revert INVALID_ERC721_AMOUNT(); will always be true which will revert WrapErc721 almost all the time.

Tools Used

Manual

Recommended Mitigation Steps

I don't think this has a specific recommended mitigation, as this solely depends on the protocol. For example;

1. The protocol can decide to add proper validation and handling for negative values returned by the getBalanceDelta function (even though it's intended) when the interaction type is WrapErc721.

OR

2. The protocol can mute GET_BALANCE_DELTA constant which is type(uint256).max for WrapErc721

OR

3. maybe just change @> if (specifiedAmount != 1) revert INVALID_ERC721_AMOUNT(); to @> if (specifiedAmount != -1) revert INVALID_ERC721_AMOUNT(); for WrapErc721 not entirely sure that makes sense.

Assessed type

Invalid Validation

[c4-pre-sort]

raymondfam marked the issue as insufficient quality report

[c4-pre-sort]

raymondfam marked the issue as duplicate of #142

[c4-judge]

0xA5DF marked the issue as not a duplicate

[c4-judge]

0xA5DF changed the severity to QA (Quality Assurance)

[c4-judge]

0xA5DF marked the issue as grade-c

[0xA5DF]

Impact seems low at best, closing due to low quantity of QAs