Skip to content

Latest commit

 

History

History
80 lines (57 loc) · 2.77 KB

048.md

File metadata and controls

80 lines (57 loc) · 2.77 KB

Vast Mahogany Fox

High

Malformed Authorization Check in Admin Acceptance Allows Privilege Escalation in Unitroller.sol

Summary

A flawed authorization check in the _acceptAdmin() function will prevent legitimate admin transfers and potentially allow unauthorized admin access, as the function checks if msg.sender is address(0) instead of validating if pendingAdmin is address(0), breaking the intended admin transfer flow.

https://github.com/sherlock-audit/2024-12-numa-audit/blob/main/Numa/contracts/lending/Unitroller.sol#L137-L138

Root Cause

In Numa/contracts/lending/Unitroller.sol#L137-L138 he admin acceptance authorization check is implemented incorrectly

if (msg.sender != pendingAdmin || msg.sender == address(0)) {
    return fail(Error.UNAUTHORIZED, FailureInfo.ACCEPT_ADMIN_PENDING_ADMIN_CHECK);
}

Internal pre-conditions

  1. Admin calls _setPendingAdmin() to assign a new pendingAdmin
  2. pendingAdmin is set to a non-zero address

External pre-conditions

The vulnerability is self-contained within the contract and does not require any external protocol conditions or state changes to be exploited.

Attack Path

  1. Admin initiates admin transfer by calling _setPendingAdmin(newAdmin)
  2. pendingAdmin is set to newAdmin address
  3. When legitimate newAdmin tries to call _acceptAdmin(): 
    // Given: msg.sender == pendingAdmin (legitimate call)
if (msg.sender != pendingAdmin || msg.sender == address(0)) {
    return fail(Error.UNAUTHORIZED, ...);
}
  4. The check will always fail for the legitimate pending admin because:
    • First condition msg.sender != pendingAdmin is false (good)
    • Second condition msg.sender == address(0) is false (good)
    • But msg.sender is being checked instead of pendingAdmin
  5. This breaks the admin transfer mechanism.

Impact

The protocol's admin transfer functionality is broken, leading to:

  1. Legitimate admin transfers being impossible to complete
  2. Potential privilege escalation if combined with other contract vulnerabilities
  3. Risk to core protocol security as this affects critical admin functions:
    • Implementation upgrades via _setPendingImplementation()
    • Admin transfers via _setPendingAdmin()
    • Any admin-protected functionality in the delegated implementation

PoC

No response

Mitigation

  1. Replace the current condition:
if (msg.sender != pendingAdmin || msg.sender == address(0)) {

With:

if (msg.sender != pendingAdmin || pendingAdmin == address(0)) {
  1. Consider implementing explicit require statements for better error handling:
require(pendingAdmin != address(0), "PendingAdmin cannot be zero address");
require(msg.sender == pendingAdmin, "Caller must be pendingAdmin");