`doMultipleInteractions()` does not check for duplicates

[c4-bot-10]

Lines of code

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

Vulnerability details

Impact

It's possible to insert the same element multiple of times in ids and interactions when calling doMultipleInteractions(). This will lead to improper calculations which might result in minting or burning more than we should.

Proof of Concept

Function doMultipleInteractions() is an external function, thus it can be called by anyone. It accepts to get two arrays as a parameters: interactions and ids. Those parameters are not validated and not checked for the duplicates. For the purpose of this PoC, we will call doMultipleInteractions() with ids array which contains the same id twice and interactions with the same interaction twice.

doMultipleInteractions() calls private function _doMultipleInteractions().

As we can verify, that function does not perform any additional check related to ids or interactions

It will create multiple of BalanceDelta() for the same id:

 for (uint256 i = 0; i < _idLength;) {
            balanceDeltas[i] = BalanceDelta(ids[i], 0);
            unchecked {
                ++i;
            }

and it will execute the same interaction twice:

(uint256 inputToken, uint256 inputAmount, uint256 outputToken, uint256 outputAmount) =
                _executeInteraction(
                    interaction, interactionType, externalContract, specifiedToken, specifiedAmount, userAddress_
                );

Based on doubled interactions and ids, deltas will be calculated twice:

(mintIds, mintAmounts, burnIds, burnAmounts) = balanceDeltas.createMintAndBurnArrays();

and since deltas won't be calculated properly, both mintIds and burnIds will be calculated differently.

As a result, by providing the same ids/interactions twice - user will be able to mint or burn more than he/she should:

  if (mintIds.length == 1) {
                // if there's only one we can just use the more semantically
                // appropriate _mint
                _mint(userAddress, mintIds[0], mintAmounts[0]);
            } else if (mintIds.length > 1) {
                // if there's more than one we use _mintBatch
                _mintBatch(userAddress, mintIds, mintAmounts);
            } // if there are none, we do nothing

            // burn the positive deltas from the user's balances
            if (burnIds.length == 1) {
                // if there's only one we can just use the more semantically
                // appropriate _burn
                _burn(userAddress, burnIds[0], burnAmounts[0]);
            } else if (burnIds.length > 1) {
                // if there's more than one we use _burnBatch
                _burnBatch(userAddress, burnIds, burnAmounts);
            } // if there are none,

Tools Used

Manual code review

Recommended Mitigation Steps

Make sure to check that both interactions and ids do not contain the same element in an array.

Assessed type

Other

[c4-pre-sort]

raymondfam marked the issue as insufficient quality report

[c4-pre-sort]

raymondfam marked the issue as duplicate of #20

[c4-judge]

0xA5DF changed the severity to QA (Quality Assurance)

[c4-judge]

0xA5DF marked the issue as grade-c

[0xA5DF]

Invalid I don't get how this will lead to an accounting error. Each interaction would increase/decrease only one balance delta, and each balance delta would lead to minting/burning accordingly.

[c4-judge]

0xA5DF marked the issue as grade-a