Closed sherlock-admin2 closed 1 year ago
1 comment(s) were left on this issue during the judging contest.
Trumpero commented:
invalid, excess tokens will be liquidity in the Pool contract. Additionally, scenario is user's mistake
The judge's whole argument that the excess tokens would just become liquidity in the Pool
CA and labeled the scenario as the user's mistake is not entirely accurate.
The flash
function is the primary function in question. It lends out tokens and expects them to be returned with a fee. The checks in place (require(balance0Before + feeQty0 <= balance0After, 'lacking feeQty0')
and require(balance1Before + feeQty1 <= balance1After, 'lacking feeQty1')
) only ensure that the minimum required amount (original amount + fee) is returned. They do not account for an overpayment scenario.
Actually, there doesn't seem to be any mechanism in the Pool
CA that would allow users to retrieve tokens that are stuck due to overpayment, which substantiates the report's claim.
The judge's comment that the excess tokens would just become liquidity in the Pool
CA is not standing in this case given the fact while these tokens do increase the balance of the CA, they do not contribute to the pool's liquidity or any other functional benefit unless explicitly coded to do so. Simply having more tokens in the CA balance does not mean they are usable liquidity.
Upon a deeper view of QtyDeltaMath.sol, LiqDeltaMath.sol, ReinvestmentMath.sol, FullMath.sol, and MathConstants.sol:
These libs contain helper functions and constants used in the main Pool
CA.
They don't directly address the overpayment issue. More specifically, FullMath.sol
provides functions for precise arithmetic, and QtyDeltaMath.sol
offers functions related to liquidity calculations. Yet, none of these functions provide a mechanism to handle overpayments or return excess tokens.
To summarize:
While a user might mistakenly overpay, it's the responsibility of the CA to handle such scenarios gracefully (mean avoid ending in locked funds). Relying on users to always make perfect txns is unrealistic. The principle of "fail early, fail loudly" in SC development means that CAs should prevent unintended behaviors.
The absence of an overpayment check and mechanism to retrieve overpaid funds can lead to the permanent loss of funds. This could be exploited either maliciously, where a nefarious actor convinces another user to overpay whose feasibility would eventually be varying according to the actual circumstances, leading to huge loss of funds, or even by accident where the user simply overpays whatever he actually owed (e.g. caller overpays an additional +33 eth against his initial loan of 333 eth, meaning the 33 ethers would be set to remain forever stuck in the Pool CA - for instance there is a reason if Uniswap implies such countermeasures, should see it as a proof that such scenarios are indeed possible in real-world.), or maybe for a matter of haste, can easily get to reflect a double-edged sword regardless of whatever was apparently innocent at a first glance.
As previously pointed out, though while UniswapV3's mechanism might seem similar, the key difference is that it handles overpayment by utilizing the excess in fee calculations and distributions, ensuring no funds get stuck.
Moreover, this should be at least a valid medium, and should therefore proceed by implementing a specific mechanism that handles these kinda scenarios such as the one employed within the UniV3 pool.
Escalate I think this should be medium as it has been judged on other contests
Escalate I think this should be medium as it has been judged on other contests
You've created a valid escalation!
To remove the escalation from consideration: Delete your comment.
You may delete or edit your escalation comment anytime before the 48-hour escalation window closes. After that, the escalation becomes final.
Disagree with the escalation. All overpaid tokens will be considered the same as fees and will be transferred to the feeTo
address (code snippet). As I mentioned before, this scenario is the user's mistake, and it will not affect anything in the pool because all excess tokens will be collected (not be locked). Therefore, this issue should be invalid.
All overpaid tokens will be considered the same as fees and will be transferred to the
feeTo
address (code snippet). As I mentioned before, this scenario is the user's mistake, and it will not affect anything in the pool because all excess tokens will be collected (not be locked). Therefore, this issue should be invalid.
As far as i can see you're referring to this snippet:
paid0 = balance0After - balance0Before;
paid1 = balance1After - balance1Before;
}
if (paid0 > 0) token0.safeTransfer(feeTo, paid0);
if (paid1 > 0) token1.safeTransfer(feeTo, paid1);
To counter on @Trumpero 's claims:
1. Overpayment is not explicitly handled:
While the judge's statement is technically accurate in that overpaid tokens will be treated as fees and transferred to the feeTo
address, this behavior is not clearly documented or intentional. The core issue is that the CA doesn't explicitly handle overpayments or provide a clear mechanism to address such scenarios. As a result, unsuspecting users may inadvertently overpay, causing their funds to be sent to the feeTo
address (Factory.sol#L37) without a clear rationale.
2. Implicit behavior is not always safe:
Relying on implicit behavior, such as treating overpayments as fees, can lead to unexpected outcomes. A savvy attacker may exploit this behavior to their advantage, especially if they have control or influence over the feeTo
address.
3. Comparison with Uniswap V3:
Uniswap V3's impl handles overpayment by using the excess in fee calculations and distributions. This is a deliberate (explicit and documented) mechanism to ensure no funds get stuck, showcasing the importance of handling overpayments.
if (paid0 > 0) {
uint8 feeProtocol0 = slot0.feeProtocol % 16;
uint256 fees0 = feeProtocol0 == 0 ? 0 : paid0 / feeProtocol0;
if (uint128(fees0) > 0) protocolFees.token0 += uint128(fees0);
feeGrowthGlobal0X128 += FullMath.mulDiv(paid0 - fees0, FixedPoint128.Q128, _liquidity);
}
if (paid1 > 0) {
uint8 feeProtocol1 = slot0.feeProtocol >> 4;
uint256 fees1 = feeProtocol1 == 0 ? 0 : paid1 / feeProtocol1;
if (uint128(fees1) > 0) protocolFees.token1 += uint128(fees1);
feeGrowthGlobal1X128 += FullMath.mulDiv(paid1 - fees1, FixedPoint128.Q128, _liquidity);
}
Deep diving into the kyberswap Pool:
The flash
function allows users to borrow tokens for a short duration, perform operations, and then return the borrowed amount with a fee. The vulnerability arises from this section:
uint256 balance0After = _poolBalToken0();
uint256 balance1After = _poolBalToken1();
require(balance0Before + feeQty0 <= balance0After, 'lacking feeQty0');
require(balance1Before + feeQty1 <= balance1After, 'lacking feeQty1');
This code only checks that the returned amount (including the fee) is at least as much as what was borrowed. It does not explicitly account for or limit overpayments.
For instance, consider a scenario with three users: Bob (an innocent user), Alice (an attacker), and Eve (who controls or either has some influence upon the feeTo
address).
- Alice convinces Bob to use the
flash
function.- Bob, either mistakenly or under misleading guidance from Alice, overpays the returned amount.
- The excess tokens are transferred to the
feeTo
address, which is controlled by Eve.- Alice and Eve, having colluded, split the overpaid funds, leaving Bob at a loss.
It's essential to understand that even if the scenario seems like a user's mistake, a CA should be designed to handle such behaviors gracefully. Leaving funds stuck or transferring them to an undesigned recipient can lead to trust issues and eventual misuse.
Moreover, should introduce an explicit check for overpayments in the flash
function. If the returned amount exceeds the borrowed amount plus fees, the excess should be returned to the user. For example:
function flash(
address recipient,
uint256 qty0,
uint256 qty1,
bytes calldata data
) external override lock {
// send all collected fees to feeTo
(address feeTo, ) = factory.feeConfiguration();
uint256 feeQty0;
uint256 feeQty1;
if (feeTo != address(0)) {
feeQty0 = (qty0 * swapFeeUnits) / C.FEE_UNITS;
feeQty1 = (qty1 * swapFeeUnits) / C.FEE_UNITS;
}
uint256 balance0Before = _poolBalToken0();
uint256 balance1Before = _poolBalToken1();
if (qty0 > 0) token0.safeTransfer(recipient, qty0);
if (qty1 > 0) token1.safeTransfer(recipient, qty1);
IFlashCallback(msg.sender).flashCallback(feeQty0, feeQty1, data);
uint256 balance0After = _poolBalToken0();
uint256 balance1After = _poolBalToken1();
require(balance0Before + feeQty0 <= balance0After, 'lacking feeQty0');
require(balance1Before + feeQty1 <= balance1After, 'lacking feeQty1');
uint256 paid0;
uint256 paid1;
unchecked {
paid0 = balance0After - balance0Before;
paid1 = balance1After - balance1Before;
}
uint256 expectedRepayment0 = qty0 + feeQty0;
uint256 expectedRepayment1 = qty1 + feeQty1;
require(paid0 >= expectedRepayment0, 'Insufficient token0 repayment');
require(paid1 >= expectedRepayment1, 'Insufficient token1 repayment');
// Handle overpayment by returning the excess to the sender
if (paid0 > expectedRepayment0) {
uint256 excess0 = paid0 - expectedRepayment0;
token0.safeTransfer(msg.sender, excess0);
paid0 = expectedRepayment0; // Update paid0 to avoid sending excess to feeTo
}
if (paid1 > expectedRepayment1) {
uint256 excess1 = paid1 - expectedRepayment1;
token1.safeTransfer(msg.sender, excess1);
paid1 = expectedRepayment1; // Update paid1 to avoid sending excess to feeTo
}
if (paid0 > 0) token0.safeTransfer(feeTo, paid0);
if (paid1 > 0) token1.safeTransfer(feeTo, paid1);
emit Flash(msg.sender, recipient, qty0, qty1, paid0, paid1);
}
Here we:
- Calculate the expected repayments as
qty0 + feeQty0
andqty1 + feeQty1
.- Check if the actual repayment (
paid0
andpaid1
) is greater than or equal to the expected repayments.- If there's an overpayment, the excess amount is sent back to the sender using
safeTransfer
.- Update
paid0
andpaid1
to ensure the correct amount is sent tofeeTo
.
This should handle the overpayment scenario and ensure that the user doesn't unintentionally overpay.
As said, though while the immediate impact of this may seem low, the risk of funds to become irretrievable and the possible misuse in deceiving users into overpaying makes this issue at least a medium severity. It's always prudent to address something that attackers will consider exploiting, no matter how small, for gain.
To reiterate on the underlying logic, the CA checks whether the balanceAfter
is greater than or equal to balanceBefore + fee
, but it doesn’t handle overpayments correctly, resulting in excess funds being unjustly trapped.
Contrary to the judge's response, this isn’t solely a user mistake. A CA should be able to handle edge cases properly and return overpaid amounts. I think the scenario where Alice convinces Bob to overpay isn’t unrealistic in defi where users may not fully comprehend the interactions.
Moreover, the judge mentioned that "the contract still ensures that no funds are stuck", which isn’t accurate.
For instance, if we look at the flash
function, we observe that the checks in place only ensure that the user has repaid at least the borrowed amount plus fees, but they do not prevent overpayment:
require(balance0Before + feeQty0 <= balance0After, 'lacking feeQty0'); require(balance1Before + feeQty1 <= balance1After, 'lacking feeQty1');
The excess funds are then transferred to the feeTo
address without any possibility of retrieval:
if (paid0 > 0) token0.safeTransfer(feeTo, paid0); if (paid1 > 0) token1.safeTransfer(feeTo, paid1);
This clearly demonstrates that the reported vulnerability is valid, and there is a lack of protection against overpayments in the CA, which can end in loss of funds on the side of legit users, should that either be intentionally or unintentionally.
While the judge argues that the issue is due to the user's mistake, though it's a must to understand that a CA's design should anticipate and handle possible user mistakes regardless, being resilient and user-friendly. A safely built CA should prevent such errors or at least handle them properly, thus avoiding any eventual loss of funds or unease.
The fact that overpayments are treated as fees and sent to the feeTo
address is neither documented nor intentional. It's an implicit behavior, which can be dangerous.
While the scenario provided, involving Alice and Bob, may seem unlikely, it's essential to also consider the possibility of more sophisticated social engineering or manipulation attacks. One could eventually deceive a user into overpaying or exploit external CAs interfacing with the Pool
to trigger overpayments, which represents a valid attack vector, increasing the severity of the issue.
Uni V3's method of handling overpayments is explicit and documented. It processes the entire repayment amount, using any overage as part of the fee and liquidity mechanisms. This ensures no funds will ever get stuck in the CA. Conversely, kyberswap’s lack of such a mechanism leads to funds being irretrievably lost, exposing a clear disparity in user protection between the two platforms.
As i mentioned earlier, for instance, after calculating the actual repayment amounts paid0
and paid1
the CA should compare them against the expected repayments and refund any excess amount to the user:
uint256 expectedRepayment0 = qty0 + feeQty0; uint256 expectedRepayment1 = qty1 + feeQty1; if (paid0 > expectedRepayment0) { uint256 excess0 = paid0 - expectedRepayment0; token0.safeTransfer(msg.sender, excess0); paid0 = expectedRepayment0; } if (paid1 > expectedRepayment1) { uint256 excess1 = paid1 - expectedRepayment1; token1.safeTransfer(msg.sender, excess1); paid1 = expectedRepayment1; } if (paid0 > 0) token0.safeTransfer(feeTo, paid0); if (paid1 > 0) token1.safeTransfer(feeTo, paid1);
While judge's stating that issue may seem like a user's mistake, the onus here is on the CA's design to prevent or handle such mistakes. The risk of funds to become irretrievable is a serious concern, and the comparison with Uniswap is valid, demonstrating that handling overpayments is a standard practice in well-designed systems. I think the previous example of Bob overpaying 33 eth in front of his initial 333 ether loan actually demonstrates that this could end up in such unnecessary, either uncomfortable scenarios that should nonetheless be avoided, thus leaving no room for dismissal and underscore the importance of addressing this issue with due diligence. Furthermore, an healthy built protocol should not punish users by permanently transferring overpaid funds to an unintended recipient. Due to this, should consider reassessing this as at least medium severity.
If handling overpayments and mistakes from users is obligatory, there will be many medium issues in other protocols. Whether to handle them or not depends on the options and choices of each protocol, so I believe this issue is of low/non-issue evidently based on the Sherlock rules of issue validity. Additionally, convincing, deceiving, or manipulating users doesn't make sense and can't be considered valid scenario because they are unrelated to KyberSwap's contracts. So I will end the discussion here and leave the final decision to Sherlock's judge.
According to Sherlock judging criteria - section VII. List of Issue categories that are not considered valid - we can notice the following statements:
- User experience and design improvement issues: Issues that cause minor inconvenience to users where there is no material loss of funds are not considered valid. Funds are temporarily stuck and can be recovered by the administrator or owner. Also, if a submission is a design opinion/suggestion without any clear indications of loss of funds is not a valid issue.
"Funds are temporarily stuck and can be recovered by the administrator or owner" - this condition is partially, yet not entirely reflecting the specifics of this finding, as if we ref back to when I explained why the assumption "the contract still ensures that no funds are stuck" is not accurate in my previous comment:
The excess funds are then transferred to the
feeTo
address without any possibility of retrieval:if (paid0 > 0) token0.safeTransfer(feeTo, paid0); if (paid1 > 0) token1.safeTransfer(feeTo, paid1);
Although the funds would technically be nothing but "temporarily" locked as there is no apparent mechanism that handles the refund process within the Pool itself, it is highly worth to point out that this should be handled regardless instead of keep weighting on a weakness that should be avoided in first place, overall if the CA specifically opts for such characteristics, i. e. some user feel offended since his funds are unjustly transferred to the feeTo
address, means even if this is documented behavior remain a very scarce handling mechanism and as such should be fixed.
"Also, if a submission is a design opinion/suggestion without any clear indications of loss of funds is not a valid issue" - not just a design suggestion as the eventually intentional/unintentional loss of funds on the side of a legit user is properly demonstrated in the comments above.
- Users sending ETH/native tokens accidentally just because a contract allows is not a valid medium/high.
I think in this case "just because a contract allows" is not a fair assumption, the point here is that legit users may nonetheless deliberately overpay than their initials - the kyberswap Pool is not a simple CA that allows for accidental scenarios where user might just theoretically think about randomly sending funds in without any specific reason, it is a CA that should handle such scenarios as well as responsibly handling its principal utility, in this specific case legit users may accidentally think about overpaying more than their initial owed amount for endless reasons, users can eventually get to concern about huge amounts of ether being unjustly transferred and consequently held from the feeTo
address, so this doesn't fall in the above mentioned rule and should be judged separately.
Likewise, if we look at section V. How to identify a medium issue:
- Causes a loss of funds but requires certain external conditions or specific states.
This would indeed cause a loss of funds under certain states, even if unintentional;
- Breaks core contract functionality, rendering the contract useless (should not be easily replaced without loss of funds) or leading to unknown potential exploits/loss of funds.
When a user is overpaying, he's breaking/misusing the flash
functionality in the kyberswap Pool so as explained this pertains with leading to possible loss of funds;
- A material loss of funds, no/minimal profit for the attacker at a considerable cost.
Also reflects this point as he would end with his funds locked or either "temporarily, yet apparently irretrievable" although still not profiting from his action.
As for a last overly important note, a particular view (more specific to Aave architecture) of this finding is referenced in the Aave audit here from OpenZeppelin, specifically the vulnerability is:
[M03] Incorrect refund address during repay
Wanna mention overall the ending sentence:
Whenever a loan is overpaid with Ether, consider returning all excess Ether to the actual repayer and not...**
The above statement, in itself renders the idea of this concern, which, in that particular case was fixed and categorized as medium, even though the issue is a bit different from this one.
In my opinion, this issue reflects the specified medium criteria as for each of the above mentioned points. So to conclude, I completely respect the argumentations risen upon @Trumpero 's point and will be looking forward to figure out on Sherlock's endmost decision.
Result: Low Unique Agree with the Lead judge that user overpaying is not a valid issue. Considering this issue a low
moneyversed
high
Insufficient validation in
flash
mechanism will result in locked funds if the caller overpays whatever he initially owedSummary
The
flash
function in thePool
contract allows for flash loans. An observed vulnerability arises from inadequate validation of repayment quantities. The consequence of this oversight is that if the caller repays more than they should, these excess funds become trapped within the contract without a mechanism for retrieval.Vulnerability Detail
Upon initiating a flash loan using the
flash
function, the contract accurately determines the fee based onswapFeeUnits
. Nonetheless, the post-callback balance verifications merely ensure that the ending balance surpasses or meets the initial balance added to the required fee. It omits a check against overpayment. Since there's no provision for returning or denying surplus tokens, this creates a scenario where tokens can be permanently stuck.https://github.com/sherlock-audit/2023-07-kyber-swap/blob/main/ks-elastic-sc/contracts/Pool.sol#L532-L537
https://github.com/sherlock-audit/2023-07-kyber-swap/blob/main/ks-elastic-sc/contracts/Pool.sol#L549-L550
More specifically:
balance0Before + feeQty0
andbalance1Before + feeQty1
).The problem is that there's no upper bound on how much larger it can be, meaning overpayments aren't accounted for. If a user overpays, the excess amount gets transferred to the contract, and unless there's a function specifically meant to withdraw it (which, according to the provided code, there isn't), it remains stuck.
In the Uniswap V3 flash function, the following lines are crucial:
https://github.com/Uniswap/v3-core/blob/d8b1c635c275d2a9450bd6a78f3fa2484fef73eb/contracts/UniswapV3Pool.sol#L813-L814
While this appears similar to the KyberSwap function, there's a critical distinction. The Uniswap V3 Pool tracks how much is paid back after the flash loan:
https://github.com/Uniswap/v3-core/blob/d8b1c635c275d2a9450bd6a78f3fa2484fef73eb/contracts/UniswapV3Pool.sol#L817-L818
Then, it distributes these funds in the following manner:
https://github.com/Uniswap/v3-core/blob/d8b1c635c275d2a9450bd6a78f3fa2484fef73eb/contracts/UniswapV3Pool.sol#L820-L831
Essentially, the amount repaid (be it the exact amount, underpayment, or overpayment) gets used in fee calculations and distributions. So, even if you overpay on a Uniswap V3 flash loan, the excess isn't just left sitting in the contract; it gets utilized, making it a different type of vulnerability (if at all) than in KyberSwap's scenario.
In the case of KyberSwap's Pool, overpayment results in tokens becoming dormant in the contract, effectively "stuck." In contrast, Uniswap V3 rather processes the entire repayment, using any overage as part of the fee and liquidity mechanisms.
Impact
Any participant, whether through intent or error, can repay more than the flash loan's total sum. This oversight would cause the excess funds to become irretrievable, thus leading to possible financial implications for users and impact the overall platform's sentiment.
Code Snippet
https://github.com/sherlock-audit/2023-07-kyber-swap/blob/main/ks-elastic-sc/contracts/Pool.sol#L524-L563
Tool used
Manual Review
Recommendation
Incorporate an exact validation method within the repayment mechanism to guarantee that only the loaned amount plus the mandatory fee is accepted, such as the one used in the
UniswapV3Pool
. Overages should automatically return to the sender to avert the trapping of funds.POC
To demonstrate the exploit:
Pool
contract on either a local testnet or a fork of the mainnet.flash
function to initiate a flash loan.flash
function. Within this callback, repay an amount exceeding the necessary repayment.Pool
contract readily accepts this overpayment.executeFlashLoan
function of theCallbackContract
is modified to not accept thepoolAddress
parameter since the address is already initialized in the constructor.