Part 2
Zaros
PerpetualsDEXFoundrySolidity
70,000 USDC
View results
Previous Next
Submission Details
Severity: low
Invalid

Lack of Owner Validation in the Builder

dayeneris1

Summary

The LiquidationKeeper contract lacks proper owner validation in the constructor and initialization process, which could potentially lead to ownership control issues in the upgrade process.

Vulnerability Details

constructor() {
_disableInitializers(); // No owner validation
}
​
function initialize(address owner, IPerpsEngine perpsEngine) external initializer {
__BaseKeeper_init(owner);
​
if (address(perpsEngine) == address(0)) {
revert Errors.ZeroInput("perpsEngine");
}
​
LiquidationKeeperStorage storage self = _getLiquidationKeeperStorage();
self.perpsEngine = perpsEngine;
}

The vulnerability exists because:

  1. No validation of owner address in initialization

  2. No two-step ownership transfer process

  3. No delay mechanism for critical ownership changes

  4. No events emitted for ownership changes

Impact

  • Potential ownership transfer to invalid addresses

  • No ability to recover from incorrect ownership settings

  • Lack of transparency in ownership changes

  • Risk of immediate ownership changes without time for community review

Tools Used

  • Manual code review

  • Slither

  • Security best practices analysis

Recommendations

  1. Implement comprehensive owner validation:

    function initialize(address owner, IPerpsEngine perpsEngine) external initializer {
    if (owner == address(0)) {
    revert Errors.ZeroInput("owner");
    }
    __BaseKeeper_init(owner);
    ​
    if (address(perpsEngine) == address(0)) {
    revert Errors.ZeroInput("perpsEngine");
    }
    ​
    LiquidationKeeperStorage storage self = _getLiquidationKeeperStorage();
    self.perpsEngine = perpsEngine;
    emit KeeperInitialized(owner, address(perpsEngine));
    }
    ​
    event KeeperInitialized(address indexed owner, address indexed perpsEngine);

2/ Add a two-step ownership transfer process:

address private _pendingOwner;
uint256 private constant OWNERSHIP_TRANSFER_DELAY = 2 days;
uint256 private _transferRequestTime;
​
function transferOwnership(address newOwner) public override onlyOwner {
if (newOwner == address(0)) {
revert Errors.ZeroInput("newOwner");
}
_pendingOwner = newOwner;
_transferRequestTime = block.timestamp;
emit OwnershipTransferInitiated(owner(), newOwner);
}
​
function acceptOwnership() external {
require(msg.sender == _pendingOwner, "Not pending owner");
require(block.timestamp >= _transferRequestTime + OWNERSHIP_TRANSFER_DELAY, "Transfer delay not met");
address oldOwner = owner();
_transferOwnership(_pendingOwner);
_pendingOwner = address(0);
_transferRequestTime = 0;
emit OwnershipTransferred(oldOwner, _pendingOwner);
}
​
event OwnershipTransferInitiated(address indexed currentOwner, address indexed pendingOwner);

3 / Add emergency recovery mechanisms:

function renounceOwnership() public override onlyOwner {
revert Errors.OperationNotAllowed("Ownership renouncement disabled");
}

These changes will enhance the security and transparency of ownership management while providing safeguards against potential ownership-related issues.

Updates

Lead Judging Commences

inallhonesty Lead Judge 5 months ago
Submission Judgement Published
Invalidated
Reason: Out of scope

Support

FAQs

Can't find an answer? Chat with us on Discord, Twitter or Linkedin.