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: 3/29
Findings: 3
Award: $5,901.25
🌟 Selected for report: 4
🚀 Solo Findings: 1
🌟 Selected for report: gzeon
gzeon
There is a price check to avoid flash loan attacks which significantly moved the price. If current price is 5% lower than the stored twap price, the liquidation will fail. This design can be dangerous as it is to openleverage's benefit to close under-collateralized position ASAP when there is a huge market drawdown. When the market keep trading downward, it is possible that the spot price keep trading 5% lower than the twap, which prevent any liquidation from happening and causing the protocol to be under-collateralized.
// Avoid flash loan if (prices.price < prices.cAvgPrice) { uint differencePriceRatio = prices.cAvgPrice.mul(100).div(prices.price); require(differencePriceRatio - 100 < maxLiquidationPriceDiffientRatio, 'MPT'); }
Instead of revert with maxLiquidationPriceDiffientRatio, use the twap price to determine if the position is healthy.
#0 - 0xleastwood
2022-02-19T11:23:43Z
From first impression, this findings seems legitimate. Can I get some more details on why it was disputed? @ColaM12
#1 - ColaM12
2022-02-21T02:23:29Z
There is always a chance to front run a flash loan transaction before trading in OpenLev. Also, see in line 196, position is considered not healthy only if all three price check failed including the twap price.
#2 - 0xleastwood
2022-02-21T06:29:35Z
It looks like only one condition would need to be satisfied for isPositionHealthy to return false as it uses || and not &&.
#3 - ColaM12
2022-02-21T07:42:47Z
Do you mean return true? All 3 price checks should fail when liquidating. But the position may still hold funds to pay off debt. by using maxLiquidationPriceDiffientRatio, under-priced-swaps can be limited . Otherwise, all remaining funds in the position could be drained from a flash loan attack which directly leads to a bad debt to lender.
#4 - 0xleastwood
2022-02-21T08:10:57Z
Ahh sorry my mistake. I misinterpreted that.
#5 - 0xleastwood
2022-02-21T08:12:17Z
I agree with the sponsor here. The issue outlined by the warden seems to be safeguarded by the two other checks in isPositionHealthy()
#6 - 0xleastwood
2022-02-21T09:01:29Z
Actually thinking about this more, I think the warden raised an issue related to the liquidations continuing to fail if the price keeps trending downward at an accelerated pace. I don't think the protocol would be able to respond to such events if this reverts.
#7 - 0xleastwood
2022-02-21T09:41:04Z
After discussion with the sponsor, we have agreed that this issue is valid. It is expected that the TWAP is only valid for 1 min. By removing this condition, there is potential for even larger security issues. So the sponsor has decided to make this a wont-fix but I'll keep the issue open as it is valid.
#8 - 0xleastwood
2022-02-21T09:41:34Z
This was an awesome find!
732.4219 USDT - $732.42
gzeon
transfer() only forward 2300 gas which may break when gas cost change in a future ETH upgrade
see: https://consensys.net/diligence/blog/2019/09/stop-using-soliditys-transfer-now/
payable(to).transfer(amount);
use call() instead
#0 - ColaM12
2022-02-06T14:19:59Z
Duplicate to #75
#1 - 0xleastwood
2022-02-19T03:00:36Z
While this issue is a duplicate of #75, the warden has failed to outline its impact on the protocol so I don't think its fair to treat the issues the same way.
#2 - 0xleastwood
2022-02-19T03:03:45Z
As such, I'll keep this as 1 (Low).
19.0749 USDT - $19.07
gzeon
LockedBalance.amount is a uint256 which is always >=0
require(_locked.amount >= 0, "Nothing to withdraw");
#0 - ColaM12
2022-02-02T19:43:29Z
Duplicate to #132
10.3004 USDT - $10.30
gzeon
Some variables could be set immutable to save gas
IERC20 public oleToken;
#0 - ColaM12
2022-02-02T19:49:23Z
Duplicate to #11
19.0749 USDT - $19.07
gzeon
Upgrade to Solidity ^0.8.4 bring gas saving and allow the use of custom errors to optimize gas usage. https://blog.soliditylang.org/2021/04/21/custom-errors/
#0 - ColaM12
2022-02-02T19:50:14Z
Duplicate to #18
28.2591 USDT - $28.26
gzeon
> 0 is less gas efficient than != 0 for uint in require condition when optimizer is enabled
Ref: https://twitter.com/GalloDaSballo/status/1485430908165443590
$ grep -Rn "> 0" ./contracts | grep require ./contracts/ControllerV1.sol:311: require(rewards > 0, 'rewards is zero'); ./contracts/ControllerV1.sol:342: require(supplyAmount > 0 || borrowAmount > 0, 'amount is less than 0'); ./contracts/ControllerV1.sol:358: require(supplyAmount > 0 || borrowAmount > 0, 'amount0 and amount1 is 0'); ./contracts/dex/bsc/UniV2ClassDex.sol:199: require(amountIn > 0, 'INSUFFICIENT_INPUT_AMOUNT'); ./contracts/dex/bsc/UniV2ClassDex.sol:200: require(reserveIn > 0 && reserveOut > 0, 'INSUFFICIENT_LIQUIDITY'); ./contracts/dex/bsc/UniV2ClassDex.sol:208: require(amountOut > 0, 'INSUFFICIENT_OUTPUT_AMOUNT'); ./contracts/dex/bsc/UniV2ClassDex.sol:209: require(reserveIn > 0 && reserveOut > 0, 'INSUFFICIENT_LIQUIDITY'); ./contracts/dex/eth/UniV2Dex.sol:197: require(amountIn > 0, 'INSUFFICIENT_INPUT_AMOUNT'); ./contracts/dex/eth/UniV2Dex.sol:198: require(reserveIn > 0 && reserveOut > 0, 'INSUFFICIENT_LIQUIDITY'); ./contracts/dex/eth/UniV2Dex.sol:206: require(amountOut > 0, 'INSUFFICIENT_OUTPUT_AMOUNT'); ./contracts/dex/eth/UniV2Dex.sol:207: require(reserveIn > 0 && reserveOut > 0, 'INSUFFICIENT_LIQUIDITY'); ./contracts/dex/eth/UniV3Dex.sol:126: require(secondsAgo > 0, "SecondsAgo must >0"); ./contracts/dex/eth/UniV3Dex.sol:165: require(amount0Delta > 0 || amount1Delta > 0); ./contracts/farming/FarmingPools.sol:96: require(amount > 0, "Cannot stake 0"); ./contracts/farming/FarmingPools.sol:104: require(amount > 0, "Cannot withdraw 0"); ./contracts/gov/GovernorAlpha.sol:248: require(proposalCount >= proposalId && proposalId > 0, "GovernorAlpha::state: invalid proposal id"); ./contracts/lib/Exponential.sol:346: require(b > 0, errorMessage); ./contracts/liquidity/LPool.sol:57: require(initialExchangeRateMantissa > 0, "Initial Exchange Rate Mantissa should be greater zero"); ./contracts/OLETokenLock.sol:46: require(amount > 0, "no amount available"); ./contracts/OLETokenLock.sol:57: require(releaseVars[beneficiary].amount > 0, 'beneficiary does not exist'); ./contracts/OLETokenLock.sol:75: require(releaseVar.amount > 0, "beneficiary does not exist"); ./contracts/OpenLevV1Lib.sol:81: require(defaultFeesRate < 10000 && insuranceRatio < 100 && defaultMarginLimit > 0 && updatePriceDiscount <= 100 ./contracts/OpenLevV1Lib.sol:112: require(feesRate < 10000 && marginLimit > 0 && dexs.length > 0, 'PRI'); ./contracts/OpenLevV1Lib.sol:297: require(borrow > 0, "BB0"); ./contracts/Reserve.sol:32: require(amount > 0, "amount is 0!"); ./contracts/test/MockTaxToken.sol:348: require(amount > 0, "Transfer amount must be greater than zero"); ./contracts/test/MockUniswapV3Factory.sol:71: require(tickSpacing > 0 && tickSpacing < 16384); ./contracts/test/MockUniswapV3Pair.sol:826:require(_liquidity > 0, 'L'); ./contracts/XOLE.sol:71: require(totalSupply > 0, "Can't share without locked OLE"); ./contracts/XOLE.sol:252: require(_value > 0, "Non zero value"); ./contracts/XOLE.sol:266: require(_value > 0, "need non - zero value"); ./contracts/XOLE.sol:267: require(_locked.amount > 0, "No existing lock found"); ./contracts/XOLE.sol:279: require(_locked.amount > 0, "Nothing is locked");
#0 - ColaM12
2022-02-02T19:46:18Z
Duplicate to #21
🌟 Selected for report: gzeon
104.6634 USDT - $104.66
gzeon
For example, the following rounding operation is safe
uint256 unlock_time = _unlock_time.div(WEEK).mul(WEEK);
to
uint256 unlock_time = (_unlock_time/WEEK)*WEEK;
🌟 Selected for report: gzeon
104.6634 USDT - $104.66
gzeon
require(to != beneficiary, 'same address') on OLETokenLock.sol L59 is redundant since it is impossible to have releaseVars[x].amount > 0 and releaseVars[x].amount == 0 at the same time.
require(releaseVars[beneficiary].amount > 0, 'beneficiary does not exist'); require(releaseVars[to].amount == 0, 'to is exist'); require(to != beneficiary, 'same address');