code-423n4 / 2024-01-renft-findings

2 stars 0 forks source link

executor can change admin #616

Closed c4-bot-2 closed 10 months ago

c4-bot-2 commented 10 months ago

Lines of code

https://github.com/re-nft/smart-contracts/blob/3ddd32455a849c3c6dc3c3aad7a33a6c9b44c291/src/Kernel.sol#L236-L245

Vulnerability details

Impact

The executor and admin are defined as two separate roles with separate permissions. The name admin implies that it has a higher level of privilege but since the executor can change the admin address, it also has admin permissions.

Proof of Concept

According to the Kernel constructor docstring the \_executor is in change of handling executions and the \_admin is in charge of granting/revoking roles.

https://github.com/re-nft/smart-contracts/blob/3ddd32455a849c3c6dc3c3aad7a33a6c9b44c291/src/Kernel.sol#L236-L245

/**
     * @dev Instantiate the kernel with executor and admin addresses.
     *
     * @param _executor Address in charge of handling kernel executions.
     * @param _admin    Address in charge of granting and revoking roles.
     */
    constructor(address _executor, address _admin) {
        executor = _executor;
        admin = _admin;
    }

On the contrary, the \_executor is also able to change the \_admin address, also giving it the ability to grant/revoke roles

https://github.com/re-nft/smart-contracts/blob/3ddd32455a849c3c6dc3c3aad7a33a6c9b44c291/src/Kernel.sol#L277-L302

function executeAction(Actions action_, address target_) external onlyExecutor {
        if (action_ == Actions.InstallModule) {
            ensureContract(target_);
            ensureValidKeycode(Module(target_).KEYCODE());
            _installModule(Module(target_));
        } else if (action_ == Actions.UpgradeModule) {
            ensureContract(target_);
            ensureValidKeycode(Module(target_).KEYCODE());
            _upgradeModule(Module(target_));
        } else if (action_ == Actions.ActivatePolicy) {
            ensureContract(target_);
            _activatePolicy(Policy(target_));
        } else if (action_ == Actions.DeactivatePolicy) {
            ensureContract(target_);
            _deactivatePolicy(Policy(target_));
        } else if (action_ == Actions.MigrateKernel) {
            ensureContract(target_);
            _migrateKernel(Kernel(target_));
        } else if (action_ == Actions.ChangeExecutor) {
            executor = target_;
        } else if (action_ == Actions.ChangeAdmin) {
            admin = target_;
        }

        emit Events.ActionExecuted(action_, target_);
    }

Tools Used

Manual Review

Recommended Mitigation Steps

Consider removing this privilege from the executor.

Assessed type

Access Control

c4-pre-sort commented 10 months ago

141345 marked the issue as sufficient quality report

c4-pre-sort commented 10 months ago

141345 marked the issue as primary issue

141345 commented 10 months ago

executor can change admin not sure if it is by design

c4-sponsor commented 10 months ago

Alec1017 (sponsor) disputed

Alec1017 commented 10 months ago

This is expected behavior of the protocol

0xean commented 10 months ago

The warden doesn't show any proof that this is not by design with the exception of stating

The name admin implies that it has a higher level of privilege

which is of course not explicit and just an interpretation of a name. Given that the executor has many broad privileges besides modifying the admin I think its likely this is intended and without further evidence will mark as QA since it could be better documented.

c4-judge commented 10 months ago

0xean changed the severity to QA (Quality Assurance)

c4-judge commented 10 months ago

0xean marked the issue as grade-c