distribute/distributeTokenToBuy does not stop when system is frozen

[code423n4]

Lines of code

https://github.com/reserve-protocol/protocol/blob/c4ec2473bbcb4831d62af55d275368e73e16b984/contracts/p1/Distributor.sol#L87-L133 https://github.com/reserve-protocol/protocol/blob/c4ec2473bbcb4831d62af55d275368e73e16b984/contracts/p1/RevenueTrader.sol#L57-L62

Vulnerability details

Impact

According to the system-design docs https://github.com/reserve-protocol/protocol/blob/c4ec2473bbcb4831d62af55d275368e73e16b984/docs/system-design.md, all interactions are disabled EXCEPT ERC20 functions + StRSR.stake if system is frozen.

But RevenueTraderP1.distributeTokenToBuy and DistributorP1.distribute do not check the system status.

Proof of Concept

When the system is exposed to unpredictable and serious vulnerabilities, it will be frozen by design. It means the core components are unsafe now. But the RevenueTraderP1.distributeTokenToBuy and DistributorP1.distribute will approve and transfer erc20 to other core components.

function distributeTokenToBuy() public {
        uint256 bal = tokenToBuy.balanceOf(address(this));
        tokenToBuy.safeApprove(address(distributor), 0);
        tokenToBuy.safeApprove(address(distributor), bal);

// == Interactions ==
for (uint256 i = 0; i < numTransfers; i++) {
    Transfer memory t = transfers[i];
    IERC20Upgradeable(address(t.erc20)).safeTransferFrom(_msgSender(), t.addrTo, t.amount);
}

It goes against the security considerations of the system design.

Tools Used

Manual review

Recommended Mitigation Steps

Add requireNotFrozen check.

Assessed type

Access Control

[0xean]

Without a broader impact statement, this is probably QA (code different than the specification)

[tbrent]

Added notTradingPausedOrFrozen modifier

[tbrent]

Agree is QA.

[c4-sponsor]

tbrent marked the issue as disagree with severity

[c4-sponsor]

tbrent marked the issue as sponsor confirmed

[c4-judge]

0xean changed the severity to QA (Quality Assurance)