QA Report

[code423n4]

QA

1. Title: Pull over push to give ownership (manager)

https://github.com/code-423n4/2022-06-notional-coop/blob/main/index-coop-notional-trade-module/contracts/protocol/SetToken.sol#L420-L426

In the current implementation, pending module claim the role by calling initializeModule() (pull). However, the way to change the state of manager (which has more critical role to give some address access to be a module) is quite simple and has security risk on it

RECOMMENDED MITIGATION STEP

I recommend to do the same thing as module to set manager role. We make the pending manager to claim the privilege

    function setPendingManager(address _manager) external onlyManager {
        require(!isLocked, "Only when unlocked");
        pendingManager = _manager; // pendingManager: new var at storage
        manager = _manager;
    }

    function claimManager()external {
        require(msg.sender == pendingManager);
    address oldManager = manager;
    address _pendingManager = pendingManager; //save gas :)
        manager = _pendingManager;
    delete pendingManager;
    emit ManagerEdited(_pendingManager, oldManager); //Trying to almost exact the same way as current implementation
}

2. Title: Better implementation on validating !isLocked

DRY, which stands for ‘don’t repeat yourself,’ is a principle of software development that aims at reducing the repetition of patterns and code duplication in favor of abstractions and avoiding redundancy. By using modifier to validate that isLocked == false we can make DRY code and it also can save deployment gas fee

Modifier isUnlocked() {
    require(!isLocked, "Only when unlocked");
    _;
}

And call the modifier if it was necessary:

     function removeModule(address _module) external onlyManager isUnlocked {//@audit-info: add isUnlocked modifier
        //@audit-info: Remove !isLocked validating line in every functions
        require(moduleStates[_module] == ISetToken.ModuleState.INITIALIZED, "Module must be added");

        IModule(_module).removeModule();

        moduleStates[_module] = ISetToken.ModuleState.NONE;

        modules.removeStorage(_module);

        emit ModuleRemoved(_module);
    }

Occurrences:

https://github.com/code-423n4/2022-06-notional-coop/blob/main/index-coop-notional-trade-module/contracts/protocol/SetToken.sol#L343 https://github.com/code-423n4/2022-06-notional-coop/blob/main/index-coop-notional-trade-module/contracts/protocol/SetToken.sol#L376 https://github.com/code-423n4/2022-06-notional-coop/blob/main/index-coop-notional-trade-module/contracts/protocol/SetToken.sol#L392 https://github.com/code-423n4/2022-06-notional-coop/blob/main/index-coop-notional-trade-module/contracts/protocol/SetToken.sol#L406 https://github.com/code-423n4/2022-06-notional-coop/blob/main/index-coop-notional-trade-module/contracts/protocol/SetToken.sol#L420

3. Title: Malicious module can make several function in SetToken.sol being DOS

https://github.com/code-423n4/2022-06-notional-coop/blob/main/index-coop-notional-trade-module/contracts/protocol/SetToken.sol#L343-L356

By set locker = msg.sender, we make situation that anyone who call the lock function is the only one who can unlock the contract (L354). Assuming the caller either a bad actor or has no ability to unlock the contract. By letting any whitelisted addresses (modules) unlock the contract is way saver

RECOMMENDED MITIGATION STEP

//@audit-info: Remove locker var
    function lock() external onlyModule {
        require(!isLocked, "Must not be locked");
        isLocked = true;
    }

    /**
     * PRIVELEGED MODULE FUNCTION. Unlocks the SetToken and clears the locker
     */
    function unlock() external onlyModule {
        require(isLocked, "Must be locked");
        isLocked = false;
    }

By doing this way we can prevent the problem by let other module call unlock()

4. Title: Owner can remove pair via editPair()

https://github.com/code-423n4/2022-06-notional-coop/blob/main/index-coop-notional-trade-module/contracts/protocol/PriceOracle.sol#L165-L172

editPair() is intended to edit the Ioracle address. If owner want to remove the oracle, he have to do it by calling removePair(). So checking the oracle param != address(0) is necessary (as well as when we try to addPair())

[ckoopmann]

These issues all reference contracts that are not in scope of the contest.