feat: Extended PM with permission owner and delegation capabilities

[nivida]

Description

Please include a summary of the change and be sure you follow the contributions rules we do provide here

Task ID: OS-?

Type of change

See the framework lifecycle in packages/contracts/docs/framework-lifecycle to decide what kind of change this pull request is.

• Bug fix (non-breaking change which fixes an issue)
• New feature (non-breaking change which adds functionality)
• Breaking change (fix or feature that would cause existing functionality to not work as expected)

Checklist:

• I have selected the correct base branch.
• I have performed a self-review of my own code.
• I have commented my code, particularly in hard-to-understand areas.
• My changes generate no new warnings.
• Any dependent changes have been merged and published in downstream modules.
• I ran all tests with success and extended them if necessary.
• I have updated the CHANGELOG.md file in the root folder.
• I have updated the DEPLOYMENT_CHECKLIST file in the root folder.
• I have updated the UPDATE_CHECKLIST file in the root folder.
• I have updated the Subgraph and added a QA URL to the description of this PR.
• I have created a follow-up task to update the Developer Portal with the changes made in this PR.

[novaknole]

1st Round ^_^

[brickpop]

_checkFlags() => _flagIncludes(...)

[brickpop]

• Please, add a comment explaining why the address(1) check
• Use a constant, rather than a literal value
• Couldn't we just check _newOwner == address(1), update the flags and early return?

BTW, why does address(1) need be be checked here?

• Why shouldn't we update the counters for 1?

[brickpop]

Suggested change

	uint256 _flags
	uint256 _delegateeFlags

[brickpop]

Suggested change

	function getPermissionData(
	function getPermissionStatus(

[brickpop]

Suggested change

	function getFlags(
	function getPermissionFlags(

[brickpop]

This seems outdated or unrelated to what the function does

[brickpop]

• Why not calling _isRoot() within the function and saving this parameter? The function is not pure anyway
• Why isn't the function view?

[brickpop]

Why can't these be read from createdDao.ROOT_PERMISSION_ID()?
We are introducing a double source of truth here

[brickpop]

Suggested change

	bytes32 constant APPLY_TARGET_PERMISSION_ID = keccak256("APPLY_TARGET_PERMISSION");
	bytes32 constant APPLY_PERMISSIONS_PERMISSION_ID = keccak256("APPLY_PERMISSIONS_PERMISSION");

[brickpop]

Suggesting a more readable implementation

    function transfer(
        address to,
        uint value
    ) public auth(ETH_TRANSFER_PERMISSION_ID) returns (bool success) {
        // Can be called directly or via execute()
        // If you call via execute(), you'll need EXECUTE_PERMISSION as well

        if (value == 0) return true;

        (success) = to.call{value: value}();
    }

    function execute(
        bytes32 _callId,
        IDAO.Action[] calldata _actions,
        uint256 _allowFailureMap
    )
        external
        override
        nonReentrant
        auth(EXECUTE_PERMISSION)
        returns (bytes[] memory execResults, uint256 failureMap)
    {
        if (_actions.length > MAX_ACTIONS) {
            revert TooManyActions();
        }

        uint256 gasBefore;
        uint256 gasAfter;
        bool hasExecutePermission = isGranted(
            address(this),
            msg.sender,
            EXECUTE_PERMISSION_ID,
            msg.data
        );

        execResults = new bytes[](_actions.length);

        for (uint256 i = 0; i < _actions.length; ) {
            Action calldata action = _actions[i];

            bool success;
            bytes memory execResult;

            // If action.data is 0 length, it's a native ETH transfer
            if (action.data.length == 0) {
                gasBefore = gasleft();

                // msg.sender will be checked against auth(ETH_TRANSFER_PERMISSION_ID) there
                success = transfer(action.to, action.value);
                execResult = bytes("");

                gasAfter = gasleft();
            } else {
                // Action call
                bool canExecute;
                bytes32 permissionIdOrSelector;

                bytes32 actionSelector = keccak256(action.data[:4]);
                (bool permissionCreated, , ) = getPermissionStatus(action.to, actionSelector);

                if (permissionCreated) {
		                // new permission system
                    canExecute = isGranted(action.to, msg.sender, actionSelector, action.data);
                    permissionIdOrSelector = actionSelector;
                } else {
		                // classic permission system
                    canExecute = hasExecutePermission;
                    permissionIdOrSelector = EXECUTE_PERMISSION_ID;
                }

                if (!canExecute) {
                    revert Unauthorized(action.to, msg.sender, permissionIdOrSelector);
                }

                // Try as a direct call
                gasBefore = gasleft();

                (success, execResult) = action.to.call{value: action.value}(action.data);

                gasAfter = gasleft();

                // Retry as a delegate call
                if (!success && action.to == address(this)) {
                    bytes4 result;

                    assembly {
                        result := mload(add(execResult, 32))
                    }

                    if (result == Unauthorized.selector || result == UnauthorizedOwner.selector) {
                        gasBefore = gasleft();
                        (success, execResult) = action.to.delegatecall(action.data);
                        gasAfter = gasleft();
                    }
                }
            }

            // Track the action result

            if (!success) {
                // Check if failure is allowed
                if (!hasBit(_allowFailureMap, uint8(i))) {
                    revert ActionFailed(i);
                }

                // Make sure that the action call did not fail because 63/64 of `gasleft()` was insufficient to execute the external call `.to.call` (see [ERC-150](https://eips.ethereum.org/EIPS/eip-150)).
                // In specific scenarios, i.e. proposal execution where the last action in the action array is allowed to fail, the account calling `execute` could force-fail this action by setting a gas limit
                // where 63/64 is insufficient causing the `.to.call` to fail, but where the remaining 1/64 gas are sufficient to successfully finish the `execute` call.
                if (gasAfter < gasBefore / 64) {
                    revert InsufficientGas();
                }

                // Store that this action failed.
                failureMap = flipBit(failureMap, uint8(i));
            }

            execResults[i] = execResult;

            unchecked {
                ++i;
            }
        }

        emit Executed({
            actor: msg.sender,
            callId: _callId,
            actions: _actions,
            allowFailureMap: _allowFailureMap,
            failureMap: failureMap,
            execResults: execResults
        });
    }