Skip to content
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

BackingManager getting blocklisted by one token will result in DoS #43

Open
c4-bot-5 opened this issue Aug 19, 2024 · 3 comments
Open
Labels
bug Something isn't working downgraded by judge Judge downgraded the risk level of this issue duplicate-33 grade-b Q-10 QA (Quality Assurance) Assets are not at risk. State handling, function incorrect as to spec, issues with clarity, syntax 🤖_primary AI based primary recommendation sufficient quality report This report is of sufficient quality

Comments

@c4-bot-5
Copy link
Contributor

Lines of code

https://github.com/code-423n4/2024-07-reserve/blob/main/contracts/p1/BackingManager.sol#L107

Vulnerability details

Description

The BackingManager takes care of the backing of its RToken by constantly balancing the amounts of the respective underlying assets with trades. However, there can only be one trade open at a time, which could lead to the DoS of the contract, if the contract were to be blocklisted by the token being out on a trade.

Proof of concept

Let's assume the rebalance function was called, and token A with a blocklist functionality would be sent to the newly created trade contract. After that, the BackingManager would be added to the blocklist of token A, therefore all transfers of this token to and from the BackingManager would revert. This would typically not be a huge problem, since if such a token were to be used, smoething like that should be expected when it comes to loss of funds.

However, the rebalance and revenueForwarding can only be done if no trades are open. Note that removing the asset from asset registry would not help. Since both trade types, be it the Dutch trade or the Gnosis trade, send tokens to the BackingManager in order for the trade to be settled, this action would be blocked by the blocklist.

Impact and likelihood

The likelihood of such an issue occuring is LOW. Since there is no other way to close trades then by settling (which transfers tokens), the impact should be considered HIGH, therefore MEDIUM severity.

Recommendation

Consider adding a functionality to force close trades callable by governance.

Assessed type

DoS

@c4-bot-5 c4-bot-5 added 2 (Med Risk) Assets not at direct risk, but function/availability of the protocol could be impacted or leak value bug Something isn't working labels Aug 19, 2024
c4-bot-7 added a commit that referenced this issue Aug 19, 2024
@c4-bot-12 c4-bot-12 added the 🤖_primary AI based primary recommendation label Aug 19, 2024
@howlbot-integration howlbot-integration bot added sufficient quality report This report is of sufficient quality duplicate-33 labels Aug 20, 2024
@c4-judge c4-judge added downgraded by judge Judge downgraded the risk level of this issue QA (Quality Assurance) Assets are not at risk. State handling, function incorrect as to spec, issues with clarity, syntax and removed 2 (Med Risk) Assets not at direct risk, but function/availability of the protocol could be impacted or leak value labels Aug 28, 2024
@c4-judge
Copy link
Contributor

thereksfour changed the severity to QA (Quality Assurance)

@c4-judge
Copy link
Contributor

thereksfour marked the issue as grade-c

@c4-judge c4-judge added grade-c unsatisfactory does not satisfy C4 submission criteria; not eligible for awards grade-b and removed grade-c unsatisfactory does not satisfy C4 submission criteria; not eligible for awards labels Aug 29, 2024
@c4-judge
Copy link
Contributor

c4-judge commented Sep 4, 2024

thereksfour marked the issue as grade-b

@C4-Staff C4-Staff reopened this Sep 4, 2024
@C4-Staff C4-Staff added the Q-10 label Sep 4, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working downgraded by judge Judge downgraded the risk level of this issue duplicate-33 grade-b Q-10 QA (Quality Assurance) Assets are not at risk. State handling, function incorrect as to spec, issues with clarity, syntax 🤖_primary AI based primary recommendation sufficient quality report This report is of sufficient quality
Projects
None yet
Development

No branches or pull requests

4 participants