StRSR.withdraw can be blocked

[code423n4]

Lines of code

https://github.com/reserve-protocol/protocol/blob/c4ec2473bbcb4831d62af55d275368e73e16b984/contracts/p1/StRSR.sol#L336

Vulnerability details

Impact

StRSR.withdraw can be blocked for user by changing basket's asset to IFFY status. User will not be able to withdraw and his funds can be seized later.

Proof of Concept

StRSR.withdraw function allows user to withdraw only, when basketHandler.isReady(). And assetRegistry.refresh will not be called each StRSR.withdraw call, because of leakyRefresh.

basketHandler.isReady() will be true, only when status is sound and enough time has passed, since status change. Each time status function is called, it checks the worst status of all assets that are in basket currently.

So it's enough to have at least one iffy asset in basket, to make isReady to be false. This can be easily done by attacker. Let's check FiatCollateral as example. There is refresh function, that can be called by anyone, which is trying to call oracle and get asset price. In case if error was catched during the call, then status of asset will be marked as iffy. So in order to make this asset to be iffy, attacker can call refresh function with amount of gas that is not enough to execute oracle call, but is enough to set status to iffy inside catch.

As result, after changing asset's status to iffy, basket handler is not ready anymore, which means that StRSR.withdraw will fail, when there is no asset registry refresh. So attacker can frontrun user's withdraw in order to block it. He can do that as many times as he wishes. As result user who wanted to leave may be seized.

Tools Used

VsCode

Recommended Mitigation Steps

The way you can solve it depends on if asset's code is something that you can control. If yes, then i would recommend to allow refresh assets only for asset registry. Otherwise, you should refresh registry for each withdraw in StRSR.

Assessed type

Error

[0xean]

So in order to make this asset to be iffy, attacker can call refresh function with amount of gas that is not enough to execute oracle call, but is enough to set status to iffy inside catch.

I do not believe this to be possible per the docs here

Notice, though, that we're _not_ going IFFY when `errData` is empty, but instead just reverting with another empty error. Why? Well, it's not very well-documented (and honestly it feels like a likely candidate for future change in the EVM), but the EVM emits a error with empty low-level data if it hits an out-of-gas error. This is an issue for us, though, because if the collateral contract goes IFFY on any out-of-gas error, then an attacker can set a collateral contract to IFFY at will, just by crafting an otherwise-legitimate transaction targeted to run out of gas during the `chainlinkFeed.price_()` call.

So, to err on the side of non-griefability, these collateral contracts allow empty errors to pass through, rather than catching them and going IFFY.

[c4-judge]

0xean marked the issue as unsatisfactory: Insufficient proof

[rvierdiiev]

@0xean i have run test with it you are correct