Open c4-bot-1 opened 10 months ago
raymondfam marked the issue as insufficient quality report
raymondfam marked the issue as duplicate of #53
0xA5DF changed the severity to QA (Quality Assurance)
0xA5DF marked the issue as grade-c
Invalid, the adapter wraps the output token, which balances it back. Balance can be negative temporarily.
Hey @0xA5DF , Thanks for your time. This report has nothing to do with Balance can be negative
or giving balance back.
The issue is _computInputAmount()
specific.
Current _computInputAmount()
logic only works with ocean native primitives, but will not work with ocean adapters that wrap external protocols. (Note: _computOutAmount()
works with both ocean native primitives and ocean adapters). This report proposes a fix that allows _computInputAmount()
to work with ocean adapters as well.
The key issue is the order of _decreaseBalanceOfPrimitive
of output token first before calling adapters to perform the swap. (1) Balance will not be negative in this call path ( Note: balance can only be negative in the context of _doMultipleInteractions()
, where balanceDelta
-delta
can be negative to account for the changes after each _doInteractions()
). In the context of a single _doInteractions()
, there is no balanceDelta
and balance can never be negative in OceanERC1155.sol's balance accounting.
_decreaseBalanceOfPrimitive
will directly try to _burn()
output token balance for an ocean adapter in OceanERC1155.sol. Ocean adapter is not supposed to have wrapped output token to burn before being called to swap. Note that the revert happens at require(fromBalance >= amount, "burn amount exceeds balance");
(OceanERC1155.sol) And this is before OceanAdapter is called, OceanAdapter will not have a chance to wrap the output token
(2) Ocean adapter is also not intended to have any wrapped input/output token balance in Ocean.sol; it will only wrap in the context of a swap and its wrapped balance will be burned by Ocean in the same transaction.
Hey @viraj124 Can I get your input on this one? It seems like intended design, but I'm not sure exactly how this part works
@0xA5DF This is expected behaviour for the adapters we at this point. haven't found a good usecase for computeInputAmount
for adapters due to the ocean v3 updates & hence it is disabled for the current adapters we have
Got it, but isn't it intended to be used in the future? How would it work in that case? Is the primitive expected to have enough balance to be burnt at the beginning of the process?
Got it, but isn't it intended to be used in the future? How would it work in that case? Is the primitive expected to have enough balance to be burnt at the beginning of the process? not for the current adapters & yes whenever we decide to use it the requirement is that the adapter should have funds to burn
Got it, I think it isn't more than a low in any case, so it doesn't change anything regarding the final score.
0xA5DF marked the issue as grade-b
0xA5DF marked the issue as grade-a
Lines of code
https://github.com/code-423n4/2023-11-shellprotocol/blob/485de7383cdf88284ee6bcf2926fb7c19e9fb257/src/ocean/Ocean.sol#L798-L801 https://github.com/code-423n4/2023-11-shellprotocol/blob/485de7383cdf88284ee6bcf2926fb7c19e9fb257/src/ocean/Ocean.sol#L1043-L1044
Vulnerability details
Impact
To swap through Ocean.sol's
_computeInputAmount()
, user can interact with Ocean natively registered LiquidityPool or external AMMs through OceanAdaptor. When_computeInputAmount()
calls to an OceanAdaptor which connects with an external liquidity pool,_computeInputAmount()
is highly vulnerable to DOS, reverting user interactions.Proof of Concept
In the context where an OceanAdaptor is used, the adaptor is meant to wrap the swap funciton of a corresponding external liquidity pool. Both
computInputAmount()
andcomputeOutputAmount()
can be used depending on what the external pool swapping methods allow. Although in OceanAdaptor.sol,computeInputAmount()
is currently disabled due to incompatibility with Curve's pool, OceanAdaptor is an abstract contract that can be overwritten to enablecomputeInputAmount()
for other external pools that are compatible with compute input amount methods.However, whenever an adaptor allows
computeInputAmount()
for an external pool, callingcomputeInputAmount()
from Ocean.sol will likely always revert, DOSsing the functionality. This is because in Ocean.sol, a primitive (in this case an adaptor)'s output token balance is always decreased first, before actually calling the primitive/adaptor'scomputeInputAmount()
. This order is problematic.The adaptor will only receive the output token from external liquidity pools after execution of
computeInputAmount
call. Before Ocean calling adaptor'scomputeInputAmount
, the adaptor will not have output tokens wrapped to be burned by Ocean.sol, this will cause a direct revert due to insufficient balance.In addition, the adaptor will not be able to receive input tokens from the Ocean to perform the external swap on the liquidity pool either. This means even though an external liquidity pool supports computing input amount, the interaction flow will always revert due to the order in which
_decreaseBalanceOfPrimitive
is ahead of the call to the adaptor and also the fact that the adaptor will not be able receive any input tokens to perform the swap because_increaseBalanceOfPrimitive
is called aftercomputeInputAmount
.(https://github.com/code-423n4/2023-11-shellprotocol/blob/485de7383cdf88284ee6bcf2926fb7c19e9fb257/src/ocean/Ocean.sol#L798-L801)
(https://github.com/code-423n4/2023-11-shellprotocol/blob/485de7383cdf88284ee6bcf2926fb7c19e9fb257/src/ocean/Ocean.sol#L1043-L1044)
Note that in
_computeInputAmount
, the only case when it makes sense to_decreaseBalanceOfPrimitive()
beforeIOceanPrimitive(primitive).computeInputAmount()
, is when the primitive is an Ocean native liquidity pool that has registered its LpToken (see sample implementation in src/mock/ConstantSum.sol), when a native liquidity pool solely relies on Ocean's wrapped token balance for accounting.So when the use case is swapping with ocean adapters that wrap an external protocol, the adapter will start with 0 balance of any output token in Ocean.sol. So whenever a user trying to either swap token or even add liquidity to the adapter, regardless of what the output token is, the interaction will always revert at
_decreaseBalanceOfPrimitive()
due to zero balance.I consider this a medium severity because adapters will never be able to execute
computeInputAmount
regardless of its own overwriting implementation or external protocol implementation. Part of Ocean's offering will be disabled.Tools Used
Manual Review
Recommended Mitigation Steps
(1) In Ocean.sol
_computeInputAmount()
, consider adding a bypass for ocean adaptors. This can be done by allowing OceanAdaptor.sol to support interface (following ERC-165) to use as an identifier of adaptor primitive contracts.(2) Then in Ocean.sol
_computeInputAmount()
, Check whether theprimitive
supports OceanAdaptor interface. If so, bypass the flow following this order:_increaseBalanceOfPrimitive()
->IOceanPrimitive(primitive).computeInputAmount()
->_decreaseBalanceOfPrimitive()
.(3) Else if the
primitive
doesn't support OceanAdaptor interface, consider it an Ocean native liquidity pool. Then follow the current order:_decreaseBalanceOfPrimitive()
->IOceanPrimitive(primitive).computeInputAmount()
->_increaseBalanceOfPrimitive()
Assessed type
Other