search

sherlock-audit / 2023-05-perennial-judging

12 stars 9 forks source link

0xmuxyz - `0` (`address(0)`) would be assigned into the `onlyOwner()` modifier on each update function, which lead to a bad situation that each parameter would never be able to updated #172

Closed sherlock-admin closed 1 year ago

sherlock-admin commented 1 year ago

0xmuxyz

medium

0 (address(0)) would be assigned into the onlyOwner() modifier on each update function, which lead to a bad situation that each parameter would never be able to updated

Summary

Within the Controller contract, the onlyOwner() modifier would be applied to each update functions.

However, within the update functions defined in the Controller contract, 0 (address(0)) would be assigned into the onlyOwner() modifier.

Due to that, within the Controller contract, every time the transaction would be reverted when each update function would be called. And therefore, each parameters that is supposed to be updated by calling the each update function would never be able to be updated.

Vulnerability Detail

Within the Controller#updateCollateral(), the caller would be checked via the onlyOwner() modifier like this: https://github.com/sherlock-audit/2023-05-perennial/blob/main/perennial-mono/packages/perennial/contracts/controller/Controller.sol#L181

    function updateCollateral(ICollateral newCollateral) public onlyOwner(0) { /// @audit

Within the onlyOwner(), the _checkOwner() would be called. And within the _checkOwner(), it would be revert if the caller would be the address besides msg.sender like this: https://github.com/OpenZeppelin/openzeppelin-contracts/blob/master/contracts/access/Ownable.sol#L36 https://github.com/OpenZeppelin/openzeppelin-contracts/blob/master/contracts/access/Ownable.sol#L51

    /**
     * @dev Throws if called by any account other than the owner.
     */
    modifier onlyOwner() {
        _checkOwner();  /// @audit
        _;
    }
    /**
     * @dev Throws if the sender is not the owner.
     */
    function _checkOwner() internal view virtual {
        require(owner() == _msgSender(), "Ownable: caller is not the owner");  /// @audit
    }

However, within the Controller#updateCollateral(), 0 (address(0)) would be assigned into the onlyOwner() modifier. Therefore, when the Controller#updateCollateral() would be called, the transaction would always be reverted. As a result, the collateral would never be updated.

By the way, within the Controller, this vulnerability would be applied to not only the Controller#updateCollateral() but also the other update functions like this:

Thus, when the updates functions above would be called, these transaction would always be reverted. As a result, the parameters that is updated by these update functions above would also never be able to be updated.

Impact

Within the Controller contract, every time the transaction would be reverted when each update function would be called. And therefore, each parameters that is supposed to be updated by calling the each update function would never be able to be updated.

Code Snippet

Tool used

Manual Review

Recommendation

For example, within the Controller#updateCollateral(), Consider just removing the argument from the onlyOwner() modifier like this: 

+   function updateCollateral(ICollateral newCollateral) public onlyOwner {
-   function updateCollateral(ICollateral newCollateral) public onlyOwner(0) {
        ...

Or, consider assigning the msg.sender instead of assigning 0 like this:

+   function updateCollateral(ICollateral newCollateral) public onlyOwner(msg.sender) {
-   function updateCollateral(ICollateral newCollateral) public onlyOwner(0) {
        ...

Then, the solution like above should be applied to other update functions in the Controller contract.