Flow
Sablier
FoundryDeFi
20,000 USDC
View results
Previous Next
Submission Details
Severity: low
Invalid

General Solidity Best Practices

hackz

Summary

The Adminable contract lacks proper initialization of the admin variable, fails to validate new admin addresses, and does not fully adhere to Solidity best practices, potentially leading to security vulnerabilities and unclear code behavior.

Finding Description

The Adminable contract has several areas where best practices are not followed:

  1. Admin Initialization: The admin address is not initialized in the constructor, which may lead to it defaulting to zero (0x0). This scenario locks out the admin functionality, as no one can call functions guarded by the onlyAdmin modifier.

  2. Transfer Admin Functionality: The transferAdmin function does not prevent setting the admin address to zero. Allowing this could permanently remove the ability to call admin-only functions, as no valid admin would exist.

These issues break fundamental security guarantees by making the contract susceptible to becoming unusable and could allow for potential denial-of-service scenarios.

Vulnerability Details

  • Missing Admin Initialization: Without a proper initialization, the admin variable defaults to zero. Consequently, functions that require admin privileges cannot be executed, effectively disabling key functionalities.

  • Zero Address Acceptance: Allowing the transfer of admin to a zero address can lock the contract out of its admin functions, leading to a situation where essential operations cannot be performed.

Impact

The impact of these issues is severe, as they can lead to a contract that is either permanently locked or operates without an admin. This could result in a denial of service, where legitimate operations cannot be executed, severely affecting the contract's usability and trustworthiness.

Proof of Concept

Consider a scenario where the Adminable contract is deployed without initializing the admin address:

// Deploying the contract without an admin address
Adminable contract = new Adminable();

After deployment, any attempt to call an onlyAdmin function will fail since msg.sender will not match the zero address.

If the transferAdmin function is invoked with a zero address:

contract.transferAdmin(address(0));

This action would result in the admin being set to zero, locking out all admin functionalities.

Recommendations

To fix the identified issues, the following changes should be made:

  1. Admin Initialization: Implement a constructor to initialize the admin variable.

  2. Zero Address Check: Add a requirement to prevent setting the admin to a zero address in the transferAdmin function.

Fixed Code Snippet

Here’s an example of how to address these issues in the Adminable contract:

/// @title Adminable
/// @notice See the documentation in {IAdminable}.
abstract contract Adminable is IAdminable {
address public override admin;
​
// Constructor to initialize the admin
constructor(address _admin) {
require(_admin != address(0), "Admin cannot be the zero address");
admin = _admin;
}
​
modifier onlyAdmin() {
if (admin != msg.sender) {
revert Errors.CallerNotAdmin({ admin: admin, caller: msg.sender });
}
_;
}
​
function transferAdmin(address newAdmin) public virtual override onlyAdmin {
require(newAdmin != address(0), "New admin cannot be the zero address");
emit IAdminable.TransferAdmin({ oldAdmin: msg.sender, newAdmin: newAdmin });
admin = newAdmin; // Update admin after emitting event
}
}

File Location

Adminable.sol

Updates

Lead Judging Commences

inallhonesty Lead Judge 12 months ago
Submission Judgement Published
Invalidated
Reason: Non-acceptable severity

Support

FAQs

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