Platform: Code4rena
Start Date: 27/01/2022
Pot Size: $75,000 USDT
Total HM: 6
Participants: 29
Period: 7 days
Judge: leastwood
Total Solo HM: 6
Id: 72
League: ETH
Rank: 1/29
Findings: 4
Award: $24,920.00
🌟 Selected for report: 6
🚀 Solo Findings: 2
🌟 Selected for report: hyh
16276.0417 USDT - $16,276.04
hyh
When OpenLev operations use a wrapped native token, the whole user withdraw is being handled with a payable.transfer() call.
This is unsafe as transfer has hard coded gas budget and can fail when the user is a smart contract. This way any programmatical usage of OpenLevV1 and LPool is at risk.
Whenever the user either fails to implement the payable fallback function or cumulative gas cost of the function sequence invoked on a native token transfer exceeds 2300 gas consumption limit the native tokens sent end up undelivered and the corresponding user funds return functionality will fail each time.
As OpenLevV1 closeTrade is affected this includes user's principal funds freeze scenario, so marking the issue as a high severity one.
OpenLevV1Lib and LPool have doTransferOut function that calls native token payable.transfer:
OpenLevV1Lib.doTransferOut
LPool.doTransferOut
LPool.doTransferOut is used in LPool redeem and borrow, while OpenLevV1Lib.doTransferOut is used in OpenLevV1 trade manipulation logic:
closeTrade
liquidate
The issues with transfer() are outlined here:
https://consensys.net/diligence/blog/2019/09/stop-using-soliditys-transfer-now/
OpenLevV1's closeTrade and liquidate as well as LPool's redeem, redeemUnderlying, borrowBehalf, repayBorrowBehalf, repayBorrowEndByOpenLev are all nonReentrant, so reentrancy isn't an issue and transfer() can be just replaced.
Using low-level call.value(amount) with the corresponding result check or using the OpenZeppelin Address.sendValue is advised:
https://github.com/OpenZeppelin/openzeppelin-contracts/blob/master/contracts/utils/Address.sol#L60
#0 - 0xleastwood
2022-02-19T03:03:29Z
Awesome find! Completely agree with the warden here. This would prevent users from calling sensitive functions which withdraw their funds in some way.
🌟 Selected for report: hyh
4882.8125 USDT - $4,882.81
hyh
The amount that OpenLevV1 will receive can be less than V3 DEX indicated as a swap result, while it is used as given for position debt repayment accounting.
This way actual funds received can be less than accounted, leaving to system funds deficit, which can be exploited by a malicious user, draining contract funds with multiple open/close with a taxed token.
In the trade.depositToken != longToken case when flashSell is used this can imply inability to send remainder funds to a user and the failure of the whole closeTrade function, the end result is a freezing of user's funds within the system.
trade.depositToken != longToken case, can be wrong repayment accounting, which will lead to a deficit if the received funds are less than DEX returned closeTradeVars.receiveAmount.
As a side effect, doTransferOut is done without balance check, so the whole position close can revert, leading to inability to close the position and freeze of user's funds this way:
I.e. if there is enough funds in the system they will be drained, if there is not enough funds, user's position close will fail.
V3 sell function doesn't check for balance change, using DEX returned amount as is:
If fee on tranfer tokens are fully in scope, do control all the accounting and amounts to be returned to a user via balance before/after calculations for DEX V3 logic as well.
#0 - 0xleastwood
2022-02-19T11:31:28Z
Awesome find. I was able to confirm that UniV3Dex.uniV3Sell() does not properly handle fee-on-transfer tokens by treating the amount received as the difference between before balance and after balance.
🌟 Selected for report: hyh
hyh
Incorrect error makes more complex troubleshooting and programmatic usage of the system
Revert message should state that too much sellToken is spent, not otherwise:
To be, for example:
require(amountIn <= maxSellAmount, 'sold too much');
#0 - 0xleastwood
2022-02-19T12:25:00Z
I think this is valid. Would you be able to confirm why this isn't the case @ColaM12 ?
#1 - ColaM12
2022-02-21T03:38:34Z
Agree, this is should be valid. my mistake.
🌟 Selected for report: hyh
1627.6042 USDT - $1,627.60
hyh
With further system development even minor change of behavior that comes with the address change (ie different albeit similar piece of code will run in the future vs tested now) may yield unexpected outcomes.
It is far more stable to have one uniform piece of code that uses manageable configuration parameters.
In general, hard coding induced errors are more probable when the approach is used in multiple places in a big enough system.
Hardcoded factory addresses are used in the Uniswap pair getters logic:
UniV2Dex:
UniV2ClassDex:
Consider using configuration variables with the corresponding reset mechanics and access controls to have the flexibility for further management of the system
#0 - ColaM12
2022-01-28T05:36:07Z
These hard codings are used for gas optimize only.
🌟 Selected for report: hyh
104.6634 USDT - $104.66
hyh
Gas is overspent on storage access
OpenLevV1 use storage link for trade data:
Types.Trade storage trade = activeTrades[msg.sender][marketId][longToken]
trade.deposited is accessed multiple times, and each time gas is spent on storage access:
Save this constant value to memory and use it everywhere until the update
🌟 Selected for report: hyh
104.6634 USDT - $104.66
hyh
Gas is overspent on calls
In the each token ordering if case it is a double call to calculate buyAmount.toAmountBeforeTax(buyTokenFeeRate):
1st:
2nd:
The value here is constant and the same for both if cases, so in fact only one call before if statement is needed
Consider saving the result of the first call to a memory variable and reusing it within all appearences in the if statement
hyh
Assert will consume all the available gas, providing no additional benefits when being used instead of require, that both returns gas and allows for error message
Two contracts and one library now use assert:
XOLE:
FarmingPools:
Exponential library:
Using assert in production isn't recommended, consider substituting it with require in all three cases
#0 - ColaM12
2022-01-30T02:32:11Z
Duplicate to #43