-
Notifications
You must be signed in to change notification settings - Fork 8
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
All reentrancy guards can be bypassed since sendingProgress and sendingProgressAaveHub variables inside _sendValue() can be reset #40
Comments
Worth checking |
GalloDaSballo marked the issue as sufficient quality report |
GalloDaSballo marked the issue as high quality report |
GalloDaSballo marked the issue as primary issue |
Good catch but we dont consider it a high since no userFunds relevant state variables are changed after sending the value. And since such a call would encapsulate the borrowrate check at the end everything works as planned and it cannot be used to block funds or extract value or drain funds or anything. Still good to add a reetrnacy check to receive function just in case UPDATE EDIT: |
seems like this doesnt endanger users:
Its basically just a flashloan without permission? |
trust1995 marked issue #228 as primary and marked this issue as a duplicate of 228 |
trust1995 marked the issue as partial-75 |
Great quality but does not unlock the full impact as in the primary. |
Lines of code
https://github.com/code-423n4/2024-02-wise-lending/blob/main/contracts/TransferHub/SendValueHelper.sol#L31
https://github.com/code-423n4/2024-02-wise-lending/blob/main/contracts/WrapperHub/AaveHelper.sol#L212
Vulnerability details
Summary
By sending
0 wei
toWiseLending.sol
orAaveHub.sol
, all reentrancy modifiers can be bypassed.Description
There are two copies of the
_sendValue()
function - one inside SendValueHelper.sol#L31 and another inside AaveHelper.sol#L212 which set either thesendingProgress
or thesendingProgressAaveHub
variable totrue
orfalse
. These values ofsendingProgress / sendingProgressAaveHub
are used by all the non-reentrancy modifiers like:These values of
sendingProgress / sendingProgressAaveHub
can be reset tofalse
during a ETH transfer transaction by sending0 wei
(or any amount) toWiseLending.sol
andAaveHub.sol
due to incomplete checks implemented by the protocol. This makes all the re-entrancy guarding modifiers used by the protocol ineffective.Toggle to view relevant code snippets
Root Cause & Attack Flows
In order to achieve reentrancy, an attacker would want to call
_sendValue()
again from inside an ongoing_sendValue()
(i.e. from the attacker'sreceive()
function, which is invoked either on L27 or L208), since this setssendingProgress / sendingProgressAaveHub
tofalse
and any subsequent calls are not blocked by the protocol. This nested call can be achieved by invoking the protocol's unprotectedreceive()
functions which themselves internally call_sendValue()
. Refer the tworeceive()
functions inside:They internally call
_sendValue()
which setssendingProgress / sendingProgressAaveHub
tofalse
at the end. Thus, sending even0 wei
to these contracts will help an attacker bypass reentrancy checks.This can be used to carry out the following 2 types of attacks -
Attack 1 (flash-deposit):
The above is not true because of the following two observations:
WiseLending.sol::withdrawExactAmountETH()
. It has two modifiers attached to it -syncPool
andhealthStateCheck
.syncPool
is triggered which looks like the following -_syncPoolBeforeCodeExecution
followed by "_
" ( which implies function's code substitution ) followed by_syncPoolAfterCodeExecution
.healthStateCheck
needs to be executed too and hence the actual order of execution becomes:_syncPoolBeforeCodeExecution
followed by_
" ( which implies function's code substitution ) followed byhealthStateCheck
followed by_syncPoolAfterCodeExecution
._newBorrowRate()
which is called inside _syncPoolAfterCodeExecution. Hence, it is actually run after the deposit/withdraw/borrow/payback has been made by the user, not before.This makes the following attack vector possible:
value utilization
andborrow rate
are at some level.Killer
) calls depositExactAmountETH{value: 100 ether} to deposit100 ether
.Killer
then calls withdrawExactAmountETH(nftId, 1 ether). This internally calls _sendValue() and it's expected that the reentrancy guards which are now in play, will protect the protocol from any reentrant calls while this transaction is in progress.receive()
function ( which got triggered as a result of the above call towithdrawExactAmountETH()
), send0 wei
toWiseLending.sol
.sendingProgress
tofalse
. Reentrancy guards will let us through now.Killer
continues inside hisreceive()
function and procures a flash-loan of1,000,000 ether
which he then deposits viadepositExactAmountETH{value: 1_000_000 ether}
. This "flash-deposit" lowers thevalue utilization
and hence theborrow rate
.Killer
now makes a call to any function he wants, for example borrowExactAmountETH() which has a reentrancy guard (orpaybackExactAmountETH
or some other function). He is not stopped.borrowExactAmountETH()
in the above step will bring the control back toKiller
's receive() function where he can callwithdrawExactAmountETH(nftId, 1_000_000 ether)
. This call will result in another nested call toKiller
's receive() function.Killer
now returns the flash loan.The attacker can also keep on using this flash-loan strategy to continuously manipulate the LASA stepping direction and grief indefinitely.
Attack 2 (flash-borrow):
A user is able to borrow beyond his allowed limit using the following path:
Killer2
) calls depositExactAmountETH{value: 100 ether} to deposit100 ether
.Killer2
is allowed to borrow a max amount of up to approximately 77 ether (a bit less than that), as per the protocol's defined limits.Killer2
calls borrowExactAmountETH(nftId, 1 ether) to borrow1 ether
. This triggers hisreceive()
function.receive()
function, send0 wei
toWiseLending.sol
. Just like before, this will setsendingProgress
tofalse
and will make the reentrancy guards useless.Killer2
callsborrowExactAmountETH(nftId, 99 ether)
which is allowed by the protocol sincehealthStateCheck
modifier of the parent call has not been executed yet! This borrow also triggers another nested call toKiller2
's receive() function.Killer2
uses this opportunity to use his borrowed100 ether
for some transaction involving arbitrage, etc. and then callspaybackExactAmountETH{value: 24 ether}(nftId)
to return the extra amount in the same transaction.Killer2
is now left with76 ether
of borrow which is within acceptable limits and hence no error is thrown by the protocol.Proof of Concept
Add this patch and then run via
forge test --fork-url mainnet -vv --mt test_t0x1c_flash
to see both the tests pass, circumventing the reentrancy guards.The patch:
MainHelper.sol
by_getValueUtilization()
fromprivate
topublic
to enable logging in our test case_getBorrowRate()
to enable logging in our test caseWisenLendingShutdown.t.sol
contract Killer
andcontract Killer2
, which are the attacker contractsIWiseLend
interfaceOutput (The values are printed with 9-decimal precision to improve readability):
Tools used
Foundry
Recommended Mitigation Steps
Add a
require
statement inside_sendValue()
which checks thatsendingProgress / sendingProgressAaveHub
isfalse
before proceeding further.File: contracts/TransferHub/SendValueHelper.sol 12: function _sendValue( 13: address _recipient, 14: uint256 _amount 15: ) 16: internal 17: { 18: if (address(this).balance < _amount) { 19: revert AmountTooSmall(); 20: } 21: + 22: require(!sendingProgress); 22: sendingProgress = true; 23: 24: ( 25: bool success 26: , 27: ) = payable(_recipient).call{ 28: value: _amount 29: }(""); 30: 31: sendingProgress = false; 32: 33: if (success == false) { 34: revert SendValueFailed(); 35: } 36: }
and
File: contracts/WrapperHub/AaveHelper.sol 196: function _sendValue( 197: address _recipient, 198: uint256 _amount 199: ) 200: internal 201: { 202: if (address(this).balance < _amount) { 203: revert InvalidValue(); 204: } 205: + 206: require(!sendingProgressAaveHub); 206: sendingProgressAaveHub = true; 207: 208: (bool success, ) = payable(_recipient).call{ 209: value: _amount 210: }(""); 211: 212: sendingProgressAaveHub = false; 213: 214: if (success == false) { 215: revert FailedInnerCall(); 216: } 217: }
Additionally, in case the protocol desires to still retain the ability to make transfers while a
_sendValue()
transaction is ongoing, then it's better to have another internal version of this function which is accessible only by the protocol and is never used to send funds to external users, thus never giving them control. Importantly, this new function shouldn't modifysendingProgress / sendingProgressAaveHub
. Let's call this new function_sendInternal()
. Then an example usage would be to replace the following calls here:and
Assessed type
Reentrancy
The text was updated successfully, but these errors were encountered: