In case if asset was unregistered right before `RevenueTrader.manageTokens` was called, then asset is stucked

[code423n4]

Lines of code

https://github.com/reserve-protocol/protocol/blob/3.0.0-rc5/contracts/p1/RevenueTrader.sol#L132 https://github.com/reserve-protocol/protocol/blob/3.0.0-rc5/contracts/p1/AssetRegistry.sol#L125

Vulnerability details

Impact

Token will stuck in the RevenueTrader, until it will be registered again. If it's not possible, token will stuck forever.

Proof of Concept

When BackingManager forwards revenue, then it goes to the RevenueTrader contract. To trade those tokens someone should call RevenueTrader.manageTokens manually. In case if asset was unregistered right before RevenueTrader.manageTokens was called for it, then this asset will be stucked inside RevenueTrader. This is because unregistering of asset, will remove it from registry, which makes it impossible to fetch info about it, as it will revert.

As result, token will sit into RevenueTrader, till it will be registered again. But in case when it's not possible(not desired) for some reason to register that token back, then there is no way to sell it or remove from the RevenueTrader.

Tools Used

VsCode

Recommended Mitigation Steps

Add ability to withdraw tokens from RevenueTrader, so governance can do that in order to swap tokens and then forward tokenToBuy to the distribution.

Assessed type

Error

[c4-sponsor]

tbrent marked the issue as disagree with severity

[tbrent]

The intention behind token de-registration it to deal with cases where a token has become known to be malicious. if a token is not malicious and governance accidentally un-registers the token, then governance can later re-register it in order to re-gain access to the token balance. We think this is QA/informational.

[c4-judge]

0xean changed the severity to QA (Quality Assurance)

[c4-judge]

0xean marked the issue as grade-a

[c4-judge]

0xean marked the issue as selected for report