Closed code423n4 closed 1 year ago
dmvt marked the issue as primary issue
dmvt marked the issue as unsatisfactory: Invalid
this only talks about the logic implementation contracts without taking into account that these contracts are only used as libraries for the pool contracts which has a re-entrancy guard
Lines of code
https://github.com/code-423n4/2022-11-paraspace/blob/c6820a279c64a299a783955749fdc977de8f0449/paraspace-core/contracts/protocol/libraries/logic/LiquidationLogic.sol#L150-L273 https://github.com/code-423n4/2022-11-paraspace/blob/c6820a279c64a299a783955749fdc977de8f0449/paraspace-core/contracts/protocol/libraries/logic/LiquidationLogic.sol#L516-L556 https://github.com/code-423n4/2022-11-paraspace/blob/c6820a279c64a299a783955749fdc977de8f0449/paraspace-core/contracts/protocol/libraries/logic/LiquidationLogic.sol#L860-L881 https://github.com/code-423n4/2022-11-paraspace/blob/c6820a279c64a299a783955749fdc977de8f0449/paraspace-core/contracts/protocol/libraries/logic/GenericLogic.sol#L188-L195
Vulnerability details
Impact
Function
executeLiquidateERC20()
is for liquidating a position if its Health Factor drops below 1. The caller (liquidator) coversliquidationAmount
amount of debt of the user getting liquidated, and receives a proportional amount of thecollateralAsset
plus a bonus to cover market risk. by this issue, attacker can liquidate his/her account(with another address) and perform reentrancy and transfer his collaterals while attacker's account has bad health factor and by doing so attacker would steal other suppliers tokens. the problem is that code don't follow check-effect-interact pattern inexecuteLiquidateERC20()
logics. before transferring debt tokens or collateral asset, code set values forsetBorrowing()
andsetUsingAsCollateral()
and then code makes external call which gives attacker this opportunity to perform reentrancy attack when protocol contracts storage is in wrong state(borrowing set as false for borrower in liquidation token while borrower still has debt). this wrong state would increase health factor of the account which would be possible to transfer other collaterals of the user while in reality user has bad health factor. (if you have question about the issue please contact me)There are other impacts too, attacker can manipulate
setBorrowing()
andsetUsingAsCollateral()
for user or his/her account and PTokens and make those values to be in wrong states(for example user has collateral but usingAsCollateral would be false or the opposite)Proof of Concept
if user had bad health factor then any other user can call liquidation function. during the call to
executeLiquidateERC20()
, if all user's debt in that liquidationToken is going to get paid contract setsuserConfig.setBorrowing(liquidationAssetReserve.id, false)
and then it makes an external call if liquidator sends ETH, contract swap required amount of ETH for wETH and sends extra amount back to liquidator(function_depositETH()
) which gives attacker to perform reentrancy attack. as borrowing has been set to false for thatliquidationAssetReserve.id
so health factor of user would increase, and it would be possible to perform some action without failing because of bad health factor. there are reentrancy protection in pool contract so it won't be possible to call them but it's possible transfer other collaterals of the user because contract would calculate less debt for user and health factor would be higher than 1 so transfer of collaterals would be possible and transfer method is in PTokens and Ntokens contracts other than Pool contract and it would be possible to call PToken.transfer() during reentrancy.This is part of
executeLiquidateERC20()
code which issue is happing:As you can see code first set the values for
isBorrowing
status of user in theliquidationAsset
's reserve andisUsingAsCollateral
status of user in thecollateralAsset
's reserve. ifliquidator
is going to pay all the debt ofborrower
in theliqudationAsset
the code would setuserConfig.setBorrowing(liquidationAssetReserve.id, false)
and ifliquidator
is going to receive all the collaterals of theborrower
the code would setuserConfig.setUsingAsCollateral(collateralReserve.id, false)
. then it calls_burnDebtTokens()
which repays borrowers debt by transferringliquidationAsset
fromliquidator
. and then it transfer collateral to liquidator. during_burnDebtTokens()
external call happens so code didn't follows check-effect-interact pattern and when the external call happens the contract state is wrong. This is_burnDebtTokens()
code:the code first calls
_depositETH()
to convertmsg.value
to WETH (if msg.value > 0) and then it would tries to transferliquidationAsset
and burn debt token of borrower. This is_depositETH()
code:As you can see when
liquidator
sends ETH code would try to convert it to wETH and send the extra amount of ETH back tomsg.sender
and it would make an external call whenliquidator
tries to paywETH
loan of borrower. This is part ofcalculateUserAccountData()
code in GenericLogic which Calculates the user data across the reserves.As you can see when a token is not used as borrowing it won't get calculated in totalDebt and it won't get considered in health factor even if user has debt in that token. so to perform this attack, attacker need to have this conditions:
executeLiquidateERC20(borrower=attackerAddress1, liquidationAsset=wETH, ETHSent=worth of 77\$, collateralAsset=USDC)
with his attackerAddress2(which is a contract) to liquidate his other address.attackerAddress1Config.setBorrowing(wETHReserve.id, false)
. this would make health factor of attackerAddress1 to be about 2 (110+100 collateral with 0.7 loan to collateral ratio and 70\$ wBTC debt) because in that moment the health factor calculating code would skip wETH and don't consider wETH debt of attackerAddress1.msg.sender
which is attackerAddress2.PTokenUSDT.transferFrom(attackerAddress1, attackerAddress2, 90)
. protocol would calculate attackerAddress1 health factor and because wETH won't be considered as debt the health factor would be ok and attackerAddress2 has spending allowance so protocol would transfer the collateral tokens.(the reentrancy guard won't work because Pool and PTokenUSDT has different address)executeLiquidateERC20()
and the code would transfer ETH worth of 70\$ and would send about 70\$ worth of USDC collaterals of attackerAddress1 to attackerAddress2.this is critical issue because anyone can steal contracts funds whenever an liquidation happens(by liquidating himself). attacker can create a contract which gives this steal-op-liquidation feature to others and users would give spending permission for attacker contract and call it whenever liquidation is possible.
There are other scenarios that attacker can make contracts to have wrong states for example isUsingAsCollateral would be false when user has collateral in that token or the reverse.
Tools Used
VIM
Recommended Mitigation Steps
follow the check-effect-interaction pattern and don't set isUsingAsCollateral and isBorrowing before transferring tokens.