search

sherlock-audit / 2023-10-mzero-judging

3 stars 2 forks source link

0xpiken - The proposer of the `setProposalFee()` has a chance to retrieve their proposal fee after the RESET event, while others do not have this opportunity. #40

Closed sherlock-admin4 closed 5 months ago

sherlock-admin4 commented 5 months ago

0xpiken

medium

The proposer of the setProposalFee() has a chance to retrieve their proposal fee after the RESET event, while others do not have this opportunity.

Summary

When RESET event happens, PowerToken, StandardGovernor and EmergencyGovernor will be redeployed. Certora identified one problem of proposalFee:

Proposal fee is not returned to proposer after RESET event

The sponsor stated in Audit review that it is by design.

StandardGovernor supports five types of proposals:

However, the proposal setProposalFee might have chance to be executed successfully and return the proposal fee to the proposer while other proposals will revert.

Vulnerability Detail

When a proposal in StandardGovernor succeeds, any one can call StandardGovernor#execute() to execute the succeeded proposal. Once executed, the proposal fee will be returned to the proposer. Let's take a look how addToList proposal is executed: First, addToList() can only be called by StandardGovernor itself:

    function addToList(bytes32 list_, address account_) external onlySelf {
        _addToList(list_, account_);
    }

Then, registrar#addToList() will be called to finish the rest:

    function _addToList(bytes32 list_, address account_) internal {
        IRegistrar(registrar).addToList(list_, account_);
    }

The function registrar#addToList() can only be accessed by StandardGovernor or EmergencyGovernor. Either StandardGovernor or EmergencyGovernor will be always the lastDeploy() of its deployer:

    function addToList(bytes32 list_, address account_) external onlyStandardOrEmergencyGovernor {
        _valueAt[_getIsInListKey(list_, account_)] = bytes32(uint256(1));

        emit AddressAddedToList(list_, account_);
    }
    modifier onlyStandardOrEmergencyGovernor() {
        _revertIfNotStandardOrEmergencyGovernor();
        _;
    }
    function _revertIfNotStandardOrEmergencyGovernor() internal view {
        if (msg.sender != standardGovernor() && msg.sender != emergencyGovernor()) {
            revert NotStandardOrEmergencyGovernor();
        }
    }
    function emergencyGovernor() public view returns (address) {
        return IEmergencyGovernorDeployer(emergencyGovernorDeployer).lastDeploy();
    }

    /// @inheritdoc IRegistrar
    function standardGovernor() public view returns (address) {
        return IStandardGovernorDeployer(standardGovernorDeployer).lastDeploy();
    }

In case of RESET event, the proposals in old StandardGovernor should fail to execute due to access control:

However, setProposalFee() doesn't have to access any call in registrar. It can be executed without any restrictions:

    function setProposalFee(uint256 newProposalFee_) external onlySelfOrEmergencyGovernor {
        _setProposalFee(newProposalFee_);
    }
    function _setProposalFee(uint256 newProposalFee_) internal {
        emit ProposalFeeSet(proposalFee = newProposalFee_);
    }

Copy below codes to resetToPowerHolders.t.soll and run forge test --match-test test_ExecuteOldProposalAfterResetToPowerHolders

    function test_ExecuteOldProposalAfterResetToPowerHolders() external {
        //@audit-info create setProposalFee proposal in StandardGovernor
        address[] memory targets_ = new address[](1);
        targets_[0] = address(_standardGovernor);

        uint256[] memory values_ = new uint256[](1);

        bytes[] memory callDatas_ = new bytes[](1);
        callDatas_[0] = abi.encodeWithSelector(_standardGovernor.setProposalFee.selector, 1);
        _warpToNextTransferEpoch();
        vm.prank(_dave);
        uint standProposalId = _standardGovernor.propose(targets_, values_, callDatas_, "");
        _warpToNextVoteEpoch();
        vm.prank(_alice);
        _standardGovernor.castVote(standProposalId, 1);

        //@audit-info create resetToPowerHolders proposal in ZeroGovernor
        address[] memory targetsZeroGovernor_ = new address[](1);
        targetsZeroGovernor_[0] = address(_zeroGovernor);

        bytes[] memory callDatasZeroGovernor_ = new bytes[](1);
        callDatasZeroGovernor_[0] = abi.encodeWithSelector(_zeroGovernor.resetToPowerHolders.selector);

        string memory description_ = "Reset to Power holders";

        vm.prank(_dave);
        uint256 proposalId_ = _zeroGovernor.propose(targetsZeroGovernor_, values_, callDatasZeroGovernor_, description_);

        vm.prank(_dave);
        _zeroGovernor.castVote(proposalId_, 1);

        address nextStandardGovernor_ = IStandardGovernorDeployer(_registrar.standardGovernorDeployer()).nextDeploy();

        _zeroGovernor.execute(targetsZeroGovernor_, values_, callDatasZeroGovernor_, keccak256(bytes(description_)));
        //@audit-info RESET succeeded and new standardGovernor is deployed.
        assertEq(_registrar.standardGovernor(), nextStandardGovernor_);
        //@audit-info setProposalFee proposal in old StandardGovernor can be executed successfully and the proposal fee is return to the proposer dave
        _warpToNextEpoch();
        vm.expectEmit();
        emit IGovernor.ProposalExecuted(standProposalId);
        vm.expectEmit();
        emit IStandardGovernor.ProposalFeeSet(1);
        vm.expectEmit();
        emit IERC20.Transfer(address(_standardGovernor), _dave, _standardProposalFee);
        _standardGovernor.execute(targets_, values_, callDatas_, keccak256(bytes("")));   
    }

Impact

When a RESET event happens, the response mechanism for unexpired proposals in old StandardGovernor lacks consistency, resulting in the design being broken.

Code Snippet

https://github.com/sherlock-audit/2023-10-mzero/blob/main/ttg/src/StandardGovernor.sol#L225-L227

Tool used

Manual Review

Recommendation

The design should remain consistent. Either all proposal fees should be sent to the vault, or they should be returned to the proposers.

deluca-mike commented 5 months ago

While true, I'd argue that this is of low severity, or actually not an issue at all because:

If anything, the whitepaper needs updating for more detail, because that sentence was added as an FAQ, rather than act as a detailed statement of absolute mechanics of the contracts.

toninorair commented 5 months ago

This is a design decision and tradeoff. Reset is a very serious, rare, and fundamental event. Also, this situation won't lead to loss of funds, so it cannot be classified as a Sherlock issue.

nevillehuang commented 5 months ago

@deluca-mike @toninorair I believe this issue highlights a correct inconsistency during the time of the audit. Based on the information provided, proposal fees are not supposed to be returned after RESET events, but the watson highlighted an edge case scenario that allows it to. So I am inclined to keep medium severity.

PierrickGT commented 5 months ago

@nevillehuang since this inconsistency doesn't lead to the loss of funds, I don't think it should be labeled as medium. Low/Informational would be more appropriate.