Submitted by 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 users 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
https://github.com/code-423n4/2022-01-openleverage/blob/main/openleverage-contracts/contracts/OpenLevV1Lib.sol#L253
LPool.doTransferOut
https://github.com/code-423n4/2022-01-openleverage/blob/main/openleverage-contracts/contracts/liquidity/LPool.sol#L297
LPool.doTransferOut is used in LPool redeem and borrow, while OpenLevV1Lib.doTransferOut is used in OpenLevV1 trade manipulation logic:
closeTrade
https://github.com/code-423n4/2022-01-openleverage/blob/main/openleverage-contracts/contracts/OpenLevV1.sol#L204
https://github.com/code-423n4/2022-01-openleverage/blob/main/openleverage-contracts/contracts/OpenLevV1.sol#L215
liquidate
https://github.com/code-423n4/2022-01-openleverage/blob/main/openleverage-contracts/contracts/OpenLevV1.sol#L263
https://github.com/code-423n4/2022-01-openleverage/blob/main/openleverage-contracts/contracts/OpenLevV1.sol#L295
https://github.com/code-423n4/2022-01-openleverage/blob/main/openleverage-contracts/contracts/OpenLevV1.sol#L304
The issues with transfer() are outlined here:
https://consensys.net/diligence/blog/2019/09/stop-using-soliditys-transfer-now/
OpenLevV1s closeTrade and liquidate as well as LPools redeem, redeemUnderlying, borrowBehalf, repayBorrowBehalf, repayBorrowEndByOpenLev are all nonReentrant, so reentrancy isnt 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
ColaM12 (OpenLeverage) confirmed and resolved
0xleastwood (judge) commented:
