Staked tokens pause is not possible to be called

[hats-bug-reporter]

Github username: --
Twitter username: --
Submission hash (on-chain): 0x8403d03d97414cc2c22177914b4a63f3403f711c9d321cba93be0b092f6ad09a
Severity: high

Description:
Description
The pause functions in all staked tokens (/tokens) is not possible to be invoked because Minter contracts are designed to hold the staked tokens ownership and there is no exposed function that calls stX.pause/unpause in Minter contracts.

contract stZETA is ERC20, ERC20Burnable, Pausable, Ownable {
    constructor(
        string memory name,
        string memory symbol,
        uint8 decimals
    ) ERC20(name, symbol, decimals) {}

    function pause() public onlyOwner {
        _pause();
    }

    function unpause() public onlyOwner {
        _unpause();
    }

    function mint(address to, uint256 amount) public onlyOwner {
        _mint(to, amount);
    }

    function _beforeTokenTransfer(address from, address to, uint256 amount)
        internal
        whenNotPaused
        override
    {
        super._beforeTokenTransfer(from, to, amount);
    }
}

Attack Scenario
Describe how the vulnerability can be exploited.

Recommended Mitigation

This can be fixed by removing onlyOwner from the pause/unpause functions and restrict the function to a role (PAUSER_ROLE) that can be added to multiple addresses and have the pause logic still usable.

[0xRizwan]

stZETA is OOS

[Slavchew]

Hey @0xRizwan, thanks for the quick reply.

I just gave an example with stZETA, but the issue is that stZETA has pause/unpause with onlyOwner and since Minters will be the owners of the staked tokens pause/unpause won't be able to be invoked since in both NativeMinter and ERC20Minter it doesn't have functions which call it like this:

abstract contract BaseMinter is Ownable, ReentrancyGuard {

    uint256 public depositFee = 0; // possible fee to cover bridging costs
    uint256 public constant MAX_DEPOSIT_FEE = 500; // max deposit fee 500bp (5%)
    uint256 public constant FEE_DENOMINATOR = 10000; // fee denominator for basis points

    // Staking token
    IERC20 public stakingToken;

    constructor(address _stakingToken) {
        stakingToken = IERC20(_stakingToken);
    }

    event UpdateDepositFee(uint256 _depositFee);
    event TransferStakingTokenOwnership(address indexed _newOwner);
    event Mint(address indexed caller, address indexed receiver, uint256 amount);

    function previewDeposit(uint256 amount) public view virtual returns (uint256) {
        uint256 feeAmount = amount*depositFee/FEE_DENOMINATOR;
        uint256 netAmount = amount-feeAmount;
        return netAmount;
    }

    function updateDepositFee(uint256 newFee) public onlyOwner {
        require(newFee <= MAX_DEPOSIT_FEE, ">MaxFee");
        depositFee = newFee;
        emit UpdateDepositFee(newFee);
    }

    function transferStakingTokenOwnership(address newOwner) public onlyOwner {
        stakingToken.transferOwnership(newOwner);
        emit TransferStakingTokenOwnership(newOwner);
    }

    function mint(uint256 amount, address receiver) public onlyOwner {
        stakingToken.mint(receiver, amount);
        emit Mint(address(msg.sender), receiver, amount);
    }

+    function pause() public onlyOwner {
+        stakingToken.pause();
+   }

+    function unpause() public onlyOwner {
+        stakingToken.unpause();
+    }

}

I also specified it here - The pause functions in all staked tokens (/tokens) is not possible to be invoked because Minter contracts are designed to hold the staked tokens ownership and there is no exposed function that calls stX.pause/unpause in Minter contracts.

The fix that I gave is for the tokens itself and it is for sure better, but the actual issue is inside the Minter.sol and if the tokens are meant not to be changed this can also be fixed from the Minters where the problem is.

[0xRizwan]

@Slavchew Correct. In context of this competition scope, the pause and unpause of stROSE won't be able to call by contract owner which is supposed to be stROSEMinter contract. This would break the functionality of protocol and should be Medium severity under following contest rule:

Medium
Attacks that make essential functionality of the contracts temporarily unusable or inaccessible.

[ilzheev]

It was made intentionally – multisig owner cannot pause/unpause stX transfers (while stToken is ERC20, ERC20Burnable, Pausable, Ownable). No issues with this.

[Slavchew]

How no issue, what is the idea to inherit Pausable to expose public functions for that action, to restrict the beforeTokenTransfer and then to tell that it is intended to not be used. This wasn't specified in the readme or the details of the competition and it is also ridiculous to tell this is intended as the tokens do not have pausing and the code assume they have.

[0xRizwan]

Since sponsor confirmed its intended design to not call pause function via multisig so its non-issue.

I have also second thought on pausing of stROSE tokens if its needed. The minter contract owner can change anytime to transfer ownership of stROSE to other address and the pause functions from stROSE can be directly accessed in such case. This won't break the whole contract functionality but it would make few more steps of work for contract owner. Therefore, i agree with sponsor on this issue so marking it invalid.

[Slavchew]

Yeah, that's true, but in case of emergency, instead of instantly pause the contract and no user funds to be able to be drain, the owners should perform multisig call to change the owner of the stRose token which will need first to meet some threshold then to pause it.

It is borderline between Medium/Low but it is not invalid at all.

[ilzheev]

owners should perform multisig call to change the owner

The same owners should perform multisig call to pause the contract, in case this function is implemented.

This wasn't specified in the readme or the details of the competition and it is also ridiculous to tell this is intended as the tokens do not have pausing and the code assume they have.

stToken.sol (which is not in scope) is OpenZeppelin standard ERC20 – with ownership, pause & burn.
However, it does not mean all of these functions should be used in general (base) minters.
By default, minter cannot pause stToken txs – its intended design for general minters.

Custom minters can have such function (e.g. if it's security requirement from specific protocol in order to integrate with them), thus stToken.sol allows to pause, but by default and in default Minter.sol it's not used.

We keep this issue as invalid.