Skip to content

Security Improvements Required for FeeCharge Contract #187

Open
@CaptainLEVI-XXX

Description

@CaptainLEVI-XXX

Overview

During code review of the FeeCharge contract, several potential security improvements have been identified. While the contract correctly implements the Checks-Effects-Interactions pattern, there are some areas where additional security measures could be beneficial.

Current Implementation

The contract handles native token deposits and fee charging mechanisms with basic security measures in place. Current security features include:

Proper implementation of Checks-Effects-Interactions pattern
Basic access control for fee chargers
Balance checks before withdrawals
Input validation for withdrawal amounts

Identified Issues

  1. Use of transfer() Instead of call()

Problem:

Current implementation uses transfer() which has a fixed gas stipend of 2300 gas
This could cause transactions to fail when the recipient is a contract with complex logic

Solution:

// Replace transfer() with call()
(bool success, ) = to.call{value: amount}("");
require(success, "Transfer failed");

**2. Limited Event Emission **

Problem:

Important state changes are not logged as events
This makes it difficult to track contract activity off-chain
Affects contract transparency and monitoring capabilities

Solution:

// Add these events
event NativeTokenDeposited(address indexed user, uint256 amount, bytes32[] senderIDs);
event NativeTokenWithdrawn(address indexed user, uint256 amount);
event FeeCharged(address indexed from, address indexed to, bytes32 senderID, uint256 amount);

// Emit in respective functions
function nativeTokenDeposit(bytes32[] calldata approvedSenderIDs) external payable returns (uint256 balance) {
    // existing code...
    emit NativeTokenDeposited(to, msg.value, approvedSenderIDs);
}

**3. Constructor Input Validation **

Problem:

No validation for zero addresses in constructor
Could potentially allow invalid fee chargers to be registered

Solution:

constructor(address[] memory canChargeFee) {
    require(canChargeFee.length > 0, "Empty fee charger list");
    uint256 length = canChargeFee.length;
    for (uint256 i = 0; i < length; i++) {
        address approved = canChargeFee[i];
        require(approved != address(0), "Zero address not allowed");
        _canChargeFee[approved] = true;
    }
}

Can I raise a PR for implementing these changes ?@ufoscout @veeso

Metadata

Metadata

Assignees

No one assigned

    Labels

    No labels
    No labels

    Type

    No type

    Projects

    No projects

    Milestone

    No milestone

    Relationships

    None yet

    Development

    No branches or pull requests

    Issue actions