Owner has to perpetually pay fees to withdraw fees, resulting in dust amount left in the contract

[c4-bot-1]

Lines of code

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

Vulnerability details

Proof of Concept

The owner gets fees every time _grantFeeToOcean() is invoked, (from unwrapping ether, ERC1155, ERC20 and wrapping ERC20)

  function _grantFeeToOcean(uint256 oceanId, uint256 amount) private {
        if (amount > 0) {
            // since uint, same as (amount != 0)
            _mintWithoutSafeTransferAcceptanceCheck(owner(), oceanId, amount);
        }
    }

This fee is usually in the shToken format (except for the ERC1155 fees, which follows the ERC1155 token decimals), so when the owner wants to withdraw his fees in the ERC20 format, he will have to call unwrapERC20 as well.

    function _erc20Unwrap(address tokenAddress, uint256 amount, address userAddress, uint256 inputToken) private {
        try IERC20Metadata(tokenAddress).decimals() returns (uint8 decimals) {
>           uint256 feeCharged = _calculateUnwrapFee(amount);
            uint256 amountRemaining = amount - feeCharged;

            (uint256 transferAmount, uint256 truncated) =
                _convertDecimals(NORMALIZED_DECIMALS, decimals, amountRemaining);
            feeCharged += truncated;

>           _grantFeeToOcean(inputToken, feeCharged);

            SafeERC20.safeTransfer(IERC20(tokenAddress), userAddress, transferAmount);
            emit Erc20Unwrap(tokenAddress, transferAmount, amount, feeCharged, userAddress, inputToken);
        } catch {
            revert NO_DECIMAL_METHOD();
        }
    }

When the owner calls the ERC20Unwrap interaction, he will have to pay fees as well. This means that whenever he wants to withdraw a sum of tokens, he will have to pay fees. Then, when he wants to withdraw the fees he paid, he will have to pay fees again. This will lead to a small sum of fees left inside the contract that cannot be withdrawn.

Eg. Owner has about 100e18 of shUSDC accumulated from fees that he wants to withdraw.

• He calls _erc20Unwrap() to unwrap 100e18 shUSDC.
• Assuming unwrapFeeDivisor is 2000, the fees he has to pay is 100e18 / 2000 = 5e16
• He gets 99.95e6 USDC back but 0.05e6 USDC is left in the contract
• He calls _erc20Unwrap() to unwrap 5e16 shUSDC
• He has to pay 5e16 / 2000 fees which is 2.5e13
• He gets a small sum of USDC back but another smaller sum of USDC is left in the contract

This goes on perpetually; the owner cannot withdraw all his fees entirely.

Impact

Owner will have some fees stuck in the contract.

Tools Used

VSCode

Recommended Mitigation Steps

When calling _grantFeeToOcean(), if the msg.sender is the owner, then skip the function so that the owner don't have to pay his own fees.

  function _grantFeeToOcean(uint256 oceanId, uint256 amount) private {
>       if (amount > 0 && msg.sender != owner()) {
            // since uint, same as (amount != 0)
            _mintWithoutSafeTransferAcceptanceCheck(owner(), oceanId, amount);
        }
    }

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 #98

[c4-judge]

0xA5DF changed the severity to QA (Quality Assurance)

[c4-judge]

0xA5DF marked the issue as grade-c

[0xA5DF]

Moved to #236

[c4-judge]

0xA5DF marked the issue as grade-a