Only executor can change executor in Kernel

[c4-bot-2]

Lines of code

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

Vulnerability details

Impact

As discussed with the sponsors executor is a critical role that will be given when Kernel is being deployed. Then the only way in which the executor can be changed is through executeAction which has onlyExecutor modifier.

Kernel.sol#L252-L257

/**
     * @dev Modifier which only allows calls by an executing address.
     */
    modifier onlyExecutor() {
        if (msg.sender != executor) revert Errors.Kernel_OnlyExecutor(msg.sender);
        _;
    }

Kernel.sol#L277-L302

/**
     * @dev Executes an action on a target address.
     *
     * @param action_ Action which will be performed.
     * @param target_ Address upon which the action will operate.
     */
    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_);
    }

This can result in catastrophic consequences if the executor turns out to be malicious or if his private key is stolen.

In our discussion it was mentioned that the executor won’t be a multisig wallet, but a normal EOA.

A similar problem can be found in the C4 Moonwell audit: https://github.com/code-423n4/2023-07-moonwell-findings/issues/315

Proof of Concept

There are various actions that can be performed in order to harm the protocol and it’s users:

• Executor can upgrade modules, for example PaymentEscrow, and wipe out the internal balance which is used to keep track of the ERC20 tokens that are sent when order is fulfilled and should be settled to the users after rent is stopped. That will lead to the inability to terminate all the rentals + all the escrowed funds will be lost.

PaymentEscrow.sol#L19-L25

/**
 * @title PaymentEscrowBase
 * @notice Storage exists in its own base contract to avoid storage slot mismatch during upgrades.
 */
contract PaymentEscrowBase {
    // Keeps a record of the current token balances in the escrow.
    mapping(address token => uint256 amount) public balanceOf;

• Executor can call migrate kernel to a malicious implementation, which has even bigger impact for the protocol that the first one because all policies and modules will point to a new implementation.
• Executor can change the admin by himself and he will have 100% control over the protocol - pausing/unpausing all the contracts, stealing fees, frontrunning rental reclaiming and setting enormous fees that will take all the intended payments.

Tools Used

Manual Review

Recommended Mitigation Steps

Consider applying the following changes to remove the centralisation and ensure seamless operation of the protocol:

• Allow admin to change the executor.
• Instead of using EOAs for these roles - use multi-signature wallets.

Assessed type

Rug-Pull

[c4-pre-sort]

141345 marked the issue as duplicate of #407

[c4-pre-sort]

141345 marked the issue as not a duplicate

[c4-pre-sort]

141345 marked the issue as primary issue

[c4-pre-sort]

141345 marked the issue as sufficient quality report

[c4-sponsor]

Alec1017 (sponsor) acknowledged

[c4-sponsor]

Alec1017 marked the issue as disagree with severity

[Alec1017]

Would consider this more of a QA.

[c4-judge]

0xean marked the issue as unsatisfactory: Insufficient quality

[Slavchew]

Hey @0xean, thank you for your time.

I think this report should be reviewed as it clearly shows a way in which there is one entity in the system that is responsible for the entire protocol. Also, there was no word on what the executor account setup would be, and as we pointed out in our report, the sponsor said it would assign it to EOA.

Similarly, last week Concentric protocol was hacked, due to a social engineering attack, and the deployer key was stolen, the result was catastrophic.

The attack path is the same as this one, a single entity was holding the control over all the important functions. There is also a financial incentive for the attacker as he can effectively steal all the fees accumulated in the PaymentEscrow module. The attacks mentioned in the ProofOfConcept section are explaining what are the most bad consequences (all current lenders to lose their assets and all the rented NFTs to be locked in their wallets).

[0xean]

leaving as judged.