-
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
000000 - Not removing a token from the position assets upon an owner removing a token from the known assets will cause huge issues #282
Comments
Issue should not be duplicated to #71. I already have an issue duplicated to it and they are completely different - one is related to simply the action of removing an asset from the known assets not being very well thought out and causing stuck funds while this one is regarding the asset not being removed from the position assets of a user. |
Escalate, |
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 Admin is Trusted and in this situation can call /// @notice Remove asset from the list of tokens currrently held by the position
function removeToken(address asset) external onlyPositionManager {
positionAssets.remove(asset);
} Planning to reject the escalation and invalidate the issue. |
@cvetanovv, hi, this modifier allows only the position manager to call that function. If you take a look at the position manager, you will see that the only way to call that function is if you are authorised for a position which an owner is not, only the position owner is authorised (unless the position owner specifically authorises someone else) and the position owner is not a trusted entity, he is just a regular user. |
@z3s What do you think about this issue? The root cause is the same as the main issue: removing a token from the known assets. |
I think it's okay that they will be usable as collateral, and just stopping new deposits of that token is enough, because if admins just transfer user's collateral he would be liquidable. by fixing the root cause user can transfer the old tokens out and deposit some of supported tokens. |
Because of the Admin Input/call validation rule, this issue and #71 are invalid.
Planning to reject the escalation and leave the issue as is. |
Deleted my previous comments that I wrote as they didn't provide the information here. @cvetanovv, according to this rule, issue should be valid:
There is no scenario where this function can be used properly, thus it should be valid. The rule cited in the other issue and the issue itself both have a scenario where the function can be used properly:
However, for this issue; there is no such scenario where the function can be used properly, every single time would cause a huge issue and disruption of the protocol as assets can still be used as collateral and assets can directly be transferred to the position to increase collateral even when asset has been removed from known. |
I agree with the escalation that this issue is not a dup of #71. You can take a look at this comment: #71 (comment) This issue will be the main issue, and I will duplicate #390 , #232 , #426 , #435 , #488, and #539. The severity will be Medium because it does not meet the criteria for High: https://docs.sherlock.xyz/audits/judging/judging#iv.-how-to-identify-a-high-issue Planning to accept the escalation and duplicate #390 , #232 , #426 , #435 , #488, and #539 with this issue(#282) |
@cvetanovv - Hey, can you please take a look at issue #311 and consider it as a duplicate to this one as well, rather than #71 as it also mentions the same flow of a scenario being non-existent in which this function works properly and would disrupt the protocol in multiple ways. |
@iamandreiski Yes, it could be a duplicate of this issue. You have captured the root cause: the admin cannot remove an asset. I think #524 is also a valid dup. @z3s Can you check if other issues can be duplicated with this issue? |
I don't see how #524 can be considered a duplicate. It is exactly the same as the other issue where assets are locked when an asset is removed from known. You can also see that the proposed mitigation fixes exactly that issue and is exactly the same fix as the one in the main issue that is being invalidated. |
but there is no way to remove the oracle address from an asset in the RiskEngine contract. I have explained in the issue #214 |
PositionManager’s owner can make an asset known or unknown by calling the function toggleKnownAsset in the PositionManager contract . Now if the PositionManager owner can make an asset unknown(Before this asset was known), then this asset’s oracle address should also be removed from the RiskEngine contract. If this asset’s oracle address is not removed , then pools can be created with this asset, lender will deposit this asset, borrower will borrow this asset but borrower will not be able to withdraw this asset as the asset is not supported by the PositionManager contract. Here the bug is there is no way to remove the oracle address from an asset in the RiskEngine contract.see the issue #214 |
When a new pool is created with an asset, there is no check that the asset must be supported by the PositionManager contract. There is only check that the oracle address must exist for the asset in the RiskEngine contract.so PositionManager contract’s owner must set those assets as known which has an oracle address in the RiskEngine contract.if any asset which is supported by RiskEngine contract , but not supported by PositionManager contract, in this situation if users borrows that token , then users can’t withdraw that asset from the position.see the issue #214 |
I agree that #524 is not a duplicate of #282. Regarding #214, you can see why it is invalid, and I won't duplicate it with the others from this sponsor's comment: #71 (comment) My last comment remains with the escalation decision, and I will add #311 to it: #282 (comment) Planning to accept the escalation and duplicate #390 , #232 , #426 , #435 , #488, #539, and #311 with this issue(#282). |
@cvetanovv Thanks a lot for the prompt decision, just a small correction on the above statement (a typo in the issue numbers on the last sentence) -> Issue 311 should be duplicated, not 319. :) |
Thanks for the correction. |
Result: |
Escalations have been resolved successfully! Escalation status:
|
000000
High
Not removing a token from the position assets upon an owner removing a token from the known assets will cause huge issues
Summary
Not removing a token from the position assets upon an owner removing a token from the known assets will cause huge issues
Vulnerability Detail
A user can add a token to his position assets to be used as collateral if that token is marked as known by the owner:
That token is added to the
positionAssets
set upon callingPosition::addToken()
:positionAssets.insert(asset);
An issue arises if the owner decides to later remove a particular asset from the known assets as that asset is not being removed from that set upon that happening. Since it is not being removed from that set, that token will still be used upon calculating the value of the user's collateral. The owner might decide to counteract that by removing the oracle for that asset however that will be even more problematic as liquidations for users using that token will be impossible as they will revert when oracle is equal to address(0).
Impact
Not removing a token from the position assets upon an owner removing a token from the known assets will cause huge issues
Code Snippet
https://github.com/sherlock-audit/2024-08-sentiment-v2/blob/25a0c8aeaddec273c5318540059165696591ecfb/protocol-v2/src/PositionManager.sol#L522-L525
Tool used
Manual Review
Recommendation
Remove the token from the set upon removing a token from the known assets. However, implementing some kind of a time delay before that finalizes will be important as otherwise, some users might immediately become liquidatable.
The text was updated successfully, but these errors were encountered: