Fee on transfer tokens are not accounted for in any way

[c4-bot-6]

Lines of code

https://github.com/code-423n4/2023-11-shellprotocol/blob/main/src/ocean/Ocean.sol#L820-L842 https://github.com/code-423n4/2023-11-shellprotocol/blob/main/src/ocean/Ocean.sol#L864-L880 https://github.com/code-423n4/2023-11-shellprotocol/blob/main/src/ocean/Ocean.sol#L417-L429 https://github.com/code-423n4/2023-11-shellprotocol/blob/main/src/ocean/Ocean.sol#L554-L571

Vulnerability details

Impact

Fee on transfer or other unique tokens are not properly accounted for. This could lead to wrong accounting within The Ocean and eventual losses for the protocol.

Proof of Concept

https://github.com/code-423n4/2023-11-shellprotocol/blob/main/src/ocean/Ocean.sol#L820-L842

https://github.com/code-423n4/2023-11-shellprotocol/blob/main/src/ocean/Ocean.sol#L864-L880

https://github.com/code-423n4/2023-11-shellprotocol/blob/main/src/ocean/Ocean.sol#L417-L429

https://github.com/code-423n4/2023-11-shellprotocol/blob/main/src/ocean/Ocean.sol#L554-L571

The Ocean doesn't keep track of its balances when a user is wrapping a token. It simply trusts the input amount given and uses that to do the accounting. For example, when minting the wrapped tokens in _doInteraction() and _doMultipleInteractions(), the protocol does not in any way consider the scenario where the amount received is not what the user actually sent, it simply trusts it.

This leads to wrong accounting, when a user wraps a certain ERC20 token, the amount is received by the protocol, then a wrapped token is minted to the user. Later if they wish to unwrap, they can do so and they will receive a larger amount than what was received from the protocol (considering the total amount held in the protocol is larger than the wrapped tokens minted, otherwise it would revert with an Insufficient allowance error and lead to temporary withdrawal DOS for that token).

While the sponsor has stated that such tokens won't be supported, they haven't provided any list in the docs or enforced it in any way in the code. There is also no guarantee that adapter primitives won't attempt to use such tokens or that any of the existing tokens won't be changed/upgraded in the future.

Tools Used

Manual Analysis

Recommended Mitigation Steps

Consider supporting those tokens by always checking the balances before and after transfers.

Assessed type

Token-Transfer

[c4-pre-sort]

raymondfam marked the issue as insufficient quality report

[c4-pre-sort]

raymondfam marked the issue as duplicate of #30

[c4-judge]

0xA5DF marked the issue as unsatisfactory: Out of scope

[0xNentoR]

@0xA5DF @raymondfam Thank you for judging this! I see that the following finding about cUSDCV3 has been marked valid: https://github.com/code-423n4/2023-11-shellprotocol-findings/issues/142

My submission touches on the same issue, wrong accounting within the ocean when wrapping ERC20s, caused by unique ERC20s. I've stated that my submission applies for "Fee on transfer or other unique tokens". I've also stated that I was aware they won't support fee-on-transfer and rebasing tokens. Without a specified list of the tokens, I couldn't have guessed in any way they'd be using cUSDCV3, so I made my submission as generic as possible applying to a wide category of ERC20s. Also my recommended mitigation steps for checking the balances before and after transfers would also result in resolving the accounting issue caused by tokens like cUSDCV3 (which is also suggested in the valid issue linked above).

[0xA5DF]

142 isn't related to fee-on-transfer tokens in any way

[0xNentoR]

@0xA5DF mine isn't only about fee-on-transfer tokens either. It's about the wrong accounting that occurs from fee on transfers AND unique tokens like cUSDCV3. I touch on the exact same issue as as #142: wrong accounting due to the contract not checking its balances after transfers. I'm well aware they won't be using fee on transfers, the submission is made as generic as possible to encompass all tokens that would result in this accounting issue. The mitigation proposed in both is also the same.

[0xA5DF]

Got it, it's still not a dupe.

142 is about a very specific issue and while your general statement about 'other unique tokens' does technically include that issue - that's too general to be considered the same issue as #142.

[0xNentoR]

@0xA5DF I do agree it's more specific. However, the root cause is the same: trusting the user input and not the balance received. There isn't a specific term about cUSDCV3's behavior, hence why I've just stated "other unique tokens".

Let's imagine fee-on-transfer tokens weren't out of scope. In that case, both cUSDCV3 and fee-on-transfer tokens would have to be looked at as 1 issue, not two separate, because they are both steming from the same underlying bug.

Without a list of supported tokens provided anywhere, I don't know how I'm supposed to make guesses about what tokens they are gonna be using. Putting a category of weird/unique ERC20s under one umbrella seems like the most sensible thing to do in such case.

[0xA5DF]

1. Your issue is mainly about fee on transfer tokens, which is out of scope to begin with
2. What you put here is a general statement about 'unique tokens'. This isn't the same as pointing out to a very specific token that can cause an issue.
3. It's true that your mitigation would solve that issue as well, but that doesn't make it the same issue. What matters is the root cause, and your submission doesn't point out the very specific root cause of #142

[0xNentoR]

@0xA5DF If there are 10 different tokens with unique behaviors of their transfers that cause the contract to receive and account for less balance than the user's input, then I don't think it's practical to provide 10 different submissions, when all can be handled with one singular fix/change. Wouldn't that be considered submission farming by the rules and merged into one?

I was also aware they won't support fee on transfer tokens. I've clearly stated it in the submission but there's no better way to put it for behaviors such as cUSDCV3, hence why I've said "Fee on transfer or other unique tokens". Again, without a list of supported tokens, making wild guesses is pointless.

[0xA5DF]

then I don't think it's practical to provide 10 different submissions

Yes, but you can submit one issue and list them all. Why didn't you list the single token that's relevant and not OOS? Let's be real here.

[0xNentoR]

then I don't think it's practical to provide 10 different submissions

Yes, but you can submit one issue and list them all. Why didn't you list the single token that's relevant and not OOS? Let's be real here.

There are various unique tokens out in the wild, what if I the ones I listed all happened to be out of scope? By what you're saying, it would again be deemed invalid. I've clearly explained in the submission that any such behavior would lead to wrong accounting to the protocol. If the impact and the mitigation are the same, I don't understand why it's a separate issue. Not to mention that the sponsor wasn't aware they would be using cUSDCV3 at the time of the contest. From what it seems, they just learned about it during the judging phase when you pinged them in the issue.

[0xA5DF]

Ok, I think I made my point clear enough here, I'm not going to add any further comments