Custom redemption might revert if old assets were unregistered

[code423n4]

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

[0xean]

I believe this to be a stretch for high severity. It has several pre-conditions to end up in the proposed state and I do believe it would be entirely possible for governance to change back to the original state (USDC, USDT, DAI), so assets wouldn't be lost and the impact would more be along the lines of a temporary denial of service.

Look forward to warden and sponsor comments.

[tbrent]

@0xA5DF nice find! Thoughts on an alternative mitigation?

• Could move L438 to just after L417, so that erc20sAll never includes unregistered ERC20s
• Would probably have to cache the assets as assetsAll for re-use around L438
• Has side-effect of making the ERC20 return list never include unregistered ERC20s. Current implementation can return a 0 value for an unregistered ERC20. This is properly handled by the RToken contract, but still, nice-to-have.

[0xA5DF]

Hey @tbrent
That can work as well, the only downside I can think of is that in case there's an asset that's not registered and is repeated across different baskets - the toAsset() would be called multiple times for that asset (while under the current implementation and under the mitigation I've suggested it'll be called only once), this would cost about 300 gas units per additional call (100 for the call, 2 sloads to a warm slot inside the call itself)

[tbrent]

Hey @tbrent That can work as well, the only downside I can think of is that in case there's an asset that's not registered and is repeated across different baskets - the toAsset() would be called multiple times for that asset (while under the current implementation and under the mitigation I've suggested it'll be called only once), this would cost about 300 gas units per additional call (100 for the call, 2 sloads to a warm slot inside the call itself)

Noted, good point.

[c4-sponsor]

tbrent marked the issue as sponsor confirmed

[0xean]

@tbrent - do you care to comment on your thoughts on severity? I am leaning towards M on this, but it sounds like you believe it is correct as labeled (high).

[tbrent]

@tbrent - do you care to comment on your thoughts on severity? I am leaning towards M on this, but it sounds like you believe it is correct as labeled (high).

Correct, I think high is appropriate.

[c4-judge]

0xean marked the issue as satisfactory