SpigotLib should store whitelisted functions per revenue contract

[code423n4]

Lines of code

https://github.com/debtdao/Line-of-Credit/blob/audit/code4rena-2022-11-03/contracts/utils/SpigotLib.sol#L61-L80

Vulnerability details

Impact

Currently SpigotLib stores whitelisted functions per all revenue contracts, but it should store them for each revenue contract separately as whitelisted function in one revenue contract can be malicious in another.

Proof of Concept

When Spigot is deployed for the line, then borrower with approve of arbiter can add revenue contract to Spigot. This revenue contract should have some functions to work with it through the Spigot(transferOwnership, claimRevenue). It's not allowed to use all functions of revenue contract, because of security reasons(as it can transfer ownership or claim revenue). That's why there is ability to provide whitelisted functions using SpigotedLine.updateWhitelist function.

The problem is that when whitelisted function selector is stored it is stored for all revenue contracts, not only for specific one.
https://github.com/debtdao/Line-of-Credit/blob/audit/code4rena-2022-11-03/contracts/utils/SpigotLib.sol#L208-L213

    function updateWhitelistedFunction(SpigotState storage self, bytes4 func, bool allowed) external returns (bool) {
        if(msg.sender != self.owner) { revert CallerAccessDenied(); }
        self.whitelistedFunctions[func] = allowed;
        emit UpdateWhitelistFunction(func, allowed);
        return true;
    }

And then when SpigotLib.operate is called by operator, no matter which revenue contract he wants to call, function just should be whitelisted. If it is whitelisted then call is possible to any revenue contract.

How attacker can use it.
1.Attacker creates simple revenue contract with function that is called doSomeJob().
2.Arbiter checks that revenue contract contains transferOwnership ,claimRevenue functions that are not harmful.
3.They call addSpigot and add this revenue contract to Spigot.
4.Operator asks Arbiter to include doSomeJob() function as it is needed to do revenue.
5.After the check arbiter calls updateWhitelist and now function updateWhitelist is possible to call for every revenue contract.
6.Some time later attacker creates another contract with all valid functions and doSomeJob() function which does smth harmful(like claim revenue and mess up all calculations or transfer ownership to attacker).
7.Arbiter checks that new revenue contract contains transferOwnership ,claimRevenue functions that are not harmful.
8.They call addSpigot and add this revenue contract to Spigot.
9.Now attacker is possible to call this function on new revenue contract to break things.

Also it's possible that harmful function is provided in the first revenue contract and it's allowance to call will be whitelisted with the second revenue contract which implements that function without bad actions, just to get it whitelisted. Then when function is whitelisted, attacker calls it on first revenue contract.

Tools Used

VsCode

Recommended Mitigation Steps

Consider to have whitelisted functions per revenue contract.

[c4-judge]

dmvt marked the issue as primary issue

[c4-sponsor]

kibagateaux marked the issue as sponsor acknowledged

[kibagateaux]

definitely valid. Owner has control over contracts that can be called and the whitelisted functions so its expected they will not allow any upgradeable contracts (many security risk) and will check functions across contracts before enabling

[c4-judge]

dmvt marked the issue as satisfactory

[C4-Staff]

liveactionllama marked the issue as duplicate of #312