-
Notifications
You must be signed in to change notification settings - Fork 0
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
Yashar - Lack of oracle validation in acceptLtvUpdate
can result in a DoS for the Pool-Asset pair
#548
Comments
acceptLtvUpdate
can result in a DoS for the Pool-Asset pairacceptLtvUpdate
can result in a DoS for the Pool-Asset pair
How is this issue different from Two things: The if (ltvFor[poolId][asset] == 0) ltvUpdate = LtvUpdate({ ltv: ltv, validAfter: block.timestamp }); So there is no delay in this case. When the LTV is first being set, i.e pool has never been used before this, then Now the other case: LTV is set and pool is being used. Multiple users have borrowed from the pool. All operations are working correctly. Consider the following two exploit scenarios:
Impact: All pools that have this asset have stopped working. Second scenario (mentioned in the issue):
Impact: All pools that have this asset have stopped working. The impact happens immediately after the 2nd step. As the issue mentions the root cause to be in The issue described is not cause for the impact shown. Impact comes from Protocol owner removing oracle for an asset. It does not matter if this was done in the middle of Ltv update. Also |
Escalate. The main argument on why the following issue should be invalid in the above comment. |
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. |
The problem isn't about the admin deciding to remove an asset or not. The issue is how the LTV update should work in this case. Now, to explain further regarding both of your exploit scenarios: Let’s say the pool owner sets the LTV for the first time:
Later on, they change the LTV again:
But now, the pool owner wants to go back to USDC:
As you can see, it's not the Admin causing the DoS on the pool. What actually causes the pool to be inaccessible for 72 hours is the failure to check the oracle status when updating the LTV. Your statement about this being an admin action is correct only when the admin removes an asset, which results in an immediate DoS for the pools actively using that asset. However, this report points out that the protocol isn't preventing LTV updates to an asset that has been removed. |
I agree with the escalation. This issue fits Sherlock's call validation rule, which is that admin actions can create an issue:
The root of the issue is that the admin can remove an Oracle for an asset after an LTV update request has been made but before it has been accepted, which fits the above rule. Planning to accept the escalation and invalidate the issue. |
hey @cvetanovv, i'd like to kindly ask you to reconsider the issue. as i pointed out, there are two distinct scenarios at play here:
i believe the second scenario falls outside the scope of admin action and highlights a missing validation step that should be addressed to prevent unintended DoS. |
The second scenario won't happen if the first one isn't happening. The admin removes Oracle, which causes issues in the code, and enters the Sherlock admin rule:
If the first action is not present, we have no issue. My decision to accept the escalation remains. |
hey @cvetanovv using the logic that "the second scenario won’t happen if the first one doesn’t," we could invalidate a lot of vulnerabilities. almost every operation in a protocol stems from an admin action. by that reasoning, any vulnerability could be dismissed since the protocol’s deployment is also an admin action. the real issue here is that not checking the oracle status when updating the LTV is a logic flaw, not an admin action. it’s this flaw that allows the DoS to happen, not the admin’s behavior. |
The escalation seems valid to me. It should be a low/info. While this could be added as a redundant check, there is no strong impact-likelihood basis for this to be a medium. It should be a low due to low likelihood + dependence on admin errors. |
hey @ruvaag regarding your first statement about the "redundant check": i disagree with the "no-strong-impact" comment. i mean, if a Pool getting DoSed isn't a big deal, what is? |
This issue is invalid because it depends on some actions of the admin. Once the pool is initialized, the admin does not need to update the oracle. Even if he does and some functionality is broken, this is not valid according to Sherlock's rules:
My decision to accept the escalation and invalidate the issue remains. |
sir, if the admin removes an asset (oracle), pools using that asset will break, and yeah, that's totally an admin action. but that’s not what this report is about. the issue here is the code allowing pool owners to update the LTV to an asset that shouldn’t be supported. so how can we call this an admin action?
by that logic, this finding would be invalid too because it happens due to an admin removing an asset while positions can still use it. but it's considered valid because the issue isn't the admin's action, it’s that the positions can still use an asset that shouldn’t be supported, even though the issue stems from an admin action. |
Even if we don't consider the admin rule, there is a function getOracleFor(address asset) public view returns (address) {
address oracle = oracleFor[asset];
if (oracle == address(0)) revert RiskEngine_NoOracleFound(asset);
return oracle;
} The pool owner can call the function before calling My decision to accept the escalation remains. |
sir, we can’t justify skipping the check in the main function just because there’s a if the issue could be mitigated in this way and it was intended to be, it should have been included in the docs or in the known issues. |
Result: |
Escalations have been resolved successfully! Escalation status:
|
@WangSecurity @Evert0x @cvetanovv is it really okay to close the issue when we’re still discussing it? |
@yashar0x I have written four times about how the escalation will end. There were three weeks for discussion. Since you think the escalation is incorrect, what I wrote is also invalid, and the comment of the sponsor is invalid, I'll give Evert or Wang to review the issue. |
hey @cvetanovv
yep, i'm sure the escalation is incorrect, and i’ve addressed all the points raised by the escalator here
i’ve responded to all your comments about the validity of the finding, but each time you bring up a new challenge, which is fine. i believe your main argument is that it's an admin action, but you haven’t clarified why this finding isn’t considered an admin action, but this issue is. i explained my view here.
the sponsor originally confirmed the finding as medium, then after the escalation, he changed his mind. even then, he didn’t deny the existence of the bug, he just said it’s low, and i replied to that here. i believe i’ve provided all the necessary info for your concerns but still haven’t gotten a justified answer. it would be great if @WangSecurity and @Evert0x could take a look at the report and the whole discussion. |
The escalation was resolved because @cvetanovv has explained his decision several times without changing it from the beginning. Hence, the discussion has become unnecessarily long with repeating similar arguments. @cvetanovv is correct that the Admin action rule applies here, and the issue should be invalid based on it. If you need, I could make a more detailed answer, but it would be repeating the same point as above, so there's no point in it. |
hey @WangSecurity, thanks a lot for the feedback! |
@cvetanovv was indeed correct here, and it is my honest opinion that this rule has to apply here. The escalation was resolved because the discussion started to repeat the same arguments without changing the decision. |
The protocol team fixed this issue in the following PRs/commits: |
it hurts :\ |
Yashar
Medium
Lack of oracle validation in
acceptLtvUpdate
can result in a DoS for the Pool-Asset pairSummary
The
RiskEngine.sol
allows pool owners to request LTV updates with a 72-hour timelock. However, while therequestLtvUpdate
function checks for a valid oracle, theacceptLtvUpdate
function does not. This could lead to a situation where an LTV update is accepted after the oracle has been removed or invalidated, resulting in DoS for the Pool-Asset pair.Vulnerability Detail
Pool owners can update LTV parameters using the
requestLtvUpdate
function, which employs a 72-hour timelock before the LTV change takes effect. During the request phase, the function ensures a valid oracle is set for the asset:After the timelock, the pool owner can accept this request via the
acceptLtvUpdate
function. However, given the 72-hour delay, there is a possibility that the protocol's admin could remove or change the oracle for the asset. TheacceptLtvUpdate
function does not re-check the oracle's validity before updating the LTV:If the LTV is updated for an asset without an oracle, the
getAssetValue
function, which fetches the asset's price from the oracle, will always revert, resulting in a DoS for the given Pool-Asset pair.Impact
If the LTV is updated for an asset without an oracle, it will cause a DoS for the affected Pool-Asset pair, as any attempts to fetch the asset's value will revert.
Code Snippet
https://github.com/sherlock-audit/2024-08-sentiment-v2/blob/main/protocol-v2/src/RiskEngine.sol#L167-L187
https://github.com/sherlock-audit/2024-08-sentiment-v2/blob/main/protocol-v2/src/RiskEngine.sol#L190-L210
https://github.com/sherlock-audit/2024-08-sentiment-v2/blob/main/protocol-v2/src/RiskModule.sol#L183-L187
Tool used
Manual Review
Recommendation
Re-check the validity of the oracle for the asset upon accepting the ltv update:
The text was updated successfully, but these errors were encountered: