search

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

2 stars 0 forks source link

executeAction succeeds and emits an event for invalid actions #603

Closed c4-bot-6 closed 10 months ago

c4-bot-6 commented 10 months ago

Lines of code

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

Vulnerability details

Impact

In the case an executor attempts to execute an invalid action, the transaction will succeed and an event will be emitted when to action has taken place. This will lead to misleading information for users monitoring emitted events.

Proof of Concept

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

Revert with a custom error if the action is not valid.

Assessed type

Invalid Validation

c4-pre-sort commented 10 months ago

141345 marked the issue as sufficient quality report

141345 commented 10 months ago

wrong event, no loss

should be QA

c4-pre-sort commented 10 months ago

141345 marked the issue as primary issue

c4-sponsor commented 10 months ago

Alec1017 (sponsor) confirmed

c4-sponsor commented 10 months ago

Alec1017 (sponsor) acknowledged

c4-sponsor commented 10 months ago

Alec1017 marked the issue as disagree with severity

Alec1017 commented 10 months ago

Seems like QA is appropriate

c4-sponsor commented 10 months ago

Alec1017 (sponsor) confirmed

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