code423n4 commented 1 year ago

Lines of code

https://github.com/reserve-protocol/protocol/blob/c4ec2473bbcb4831d62af55d275368e73e16b984/contracts/p1/AssetRegistry.sol#L86-L115

Vulnerability details

Impact

Attacker can disable basket, when swapping or unregistering not basket asset.

Proof of Concept

AssetRegistry.swapRegistered and AssetRegistry.unregister are both functions that can be called by governance only. It's possible that after proposal is passed, then it can be executed by anyone. Also executor can provide as much gas with the call, as he wishes.

https://github.com/reserve-protocol/protocol/blob/c4ec2473bbcb4831d62af55d275368e73e16b984/contracts/p1/AssetRegistry.sol#L86-L96

    function swapRegistered(IAsset asset) external governance returns (bool swapped) {
        require(_erc20s.contains(address(asset.erc20())), "no ERC20 collision");

        try basketHandler.quantity{ gas: _reserveGas() }(asset.erc20()) returns (uint192 quantity) {
            if (quantity > 0) basketHandler.disableBasket(); // not an interaction
        } catch {
            basketHandler.disableBasket();
        }

        swapped = _registerIgnoringCollisions(asset);
    }

Here, reserve team is checking that after basketHandler.quantity call, function still has 900000 gas, which is needed to finish basketHandler.disableBasket call. As you can see, in case if quantity of asset in the basket is 0, then there is no need to disable basket.

However, malicious user still can do that. What he need to do is to call swapRegistered with 900000 + x gas amount. Where x will be the amount that is not enough to execute basketHandler.quantity function, which will cause out of gas error and catch block will then call basketHandler.disableBasket. As result basket will be disabled if attacker wants it.

Tools Used

VsCode

Recommended Mitigation Steps

You need to ensure that you send enough gas to basketHandler.quantity function.

Assessed type

Error

tbrent commented 1 year ago

Is this duplicate with #7?

c4-sponsor commented 1 year ago

tbrent marked the issue as sponsor confirmed

c4-judge commented 1 year ago

0xean marked the issue as duplicate of #7