Custom redemption might revert if old assets were unregistered

criticalCode4rena

Lines of code

https://github.com/reserve-protocol/protocol/blob/c4ec2473bbcb4831d62af55d275368e73e16b984/contracts/p1/BasketHandler.sol#L391-L392

Vulnerability details

quoteCustomRedemption() works under the assumption that the maximum size of the erc20sAll should be assetRegistry.size(), however there can be cases where an asset was unregistered but still exists in an old basket, making the size of the old basket greater than assetRegistry.size(). In that case the function will revert with an index out of bounds error.

Impact

Users might not be able to use redeemCustom when needed.

I think this should be considered high severity, since being able to redeem the token at all time is an essential feature for the protocol that's allowed also while frozen. Not being able to redeem can result in a depeg or in governance becoming malicious and stealing RToken collateral.

Proof of Concept

Consider the following scenario:

• RToken deployed with 0.9 USDC, 0.05 USDT, 0.05 DAI
• Governance passed a vote to change it to 0.9 DAI and 0.1 USDC and un-register USDT
• Trading is paused before execution, so the basket switch occurs but the re-balance can't be executed. Meaning the actual assets that the backing manager holds are in accordance with the old basket
• A user wants to redeem using the old basket, but custom redemption reverts

As for the revert:

• erc20sAll is created here with the length of assetRegistry.size(), which is 2 in our case.
• Then in this loop the function tries to push 3 assets into erc20sAll which will result in an index-out-of-bonds error

(the function doesn't include in the final results assets that aren't registered, but it does push them too into erc20sAll)

Recommended Mitigation Steps

Allow the user to specify the length of the array erc20sAll to avoid this revert

Assessed type

Other