c4-bot-5 commented 8 months ago

Lines of code

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

Vulnerability details

Impact

Some tokens incorporate a specific condition in their transfer functions to address the scenario where the amount is equal to type(uint256).max. In such cases, the token will only transfer the user's balance. The area where this type of behaviour would be more concerning is in Ocean._erc20Wrap function: https://github.com/code-423n4/2023-11-shellprotocol/blob/485de7383cdf88284ee6bcf2926fb7c19e9fb257/src/ocean/Ocean.sol#L819-L842

If a user interacted with the protocol with interactionType = erc20Wrap and with amount = type(uint256).max, outputAmount would be set to type(uint256).max and effectively minting this value. However, the user would only transfer his total token balance to Ocean breaking the following invariant: "During any do* call, the Ocean accurately tracks balances of the tokens involved throughout the transaction."

The most famous token that implements this feature is compound's cUSDCv3. The only reason that prevents this token from breaking this invariant is that it only has 6 decimals. This means that specifiedAmount will be subject to decimal convertion trough _convertDecimals(). The amount would be truncated and type(uint256).max would not be set as input for sadeTransferFrom.

For this attack to work the only thing it is needed is for the user to use a token that implements this feature and has 18 decimals, so he avoids decimal convertion. This could happen with the surge of a new token or with the update of the implementation of an existing one.

The impact would be serious as it would allow the malicious user to steal the Ocean's total balance of this token. This includes any amount deposited by any user.

Im submitting this issue as Medium, as it can lead to loss of funds, but it is indeed dependent on certain conditions that will allow this attack to happen. Ocean should base its accounting on the actual change in balance. Not in user inputed amount.

If the amount submitted This type of behaviour is rare, but

Proof of Concept

Attacker wraps erc20 token with this specific feature and with 18 decimals. Sets specificAmount = type(uint256).max Attacker will only transfer his total balance to Ocean. Attacker mints type(uint256).max. Another user wraps the same token to Ocean Attacker can unwrap tokens from the innocent user and steal them.

Tools Used

Manual Review

Recommended Mitigation Steps

Ocean could explicitly reject the wraping/unwraping of these types of tokens.
Ocean could base the amount that is minted/burnt on the actual change of Ocean's token balance like it's done on both primitives. This would involve checking the balance before the transfer and subtracting it to the updated token balance.

Assessed type

ERC20

c4-pre-sort commented 8 months ago

raymondfam marked the issue as insufficient quality report

c4-pre-sort commented 8 months ago

raymondfam marked the issue as duplicate of #142