Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Security Improvements Required for FeeCharge Contract #187

Open
CaptainLEVI-XXX opened this issue Nov 4, 2024 · 0 comments
Open

Security Improvements Required for FeeCharge Contract #187

CaptainLEVI-XXX opened this issue Nov 4, 2024 · 0 comments

Comments

@CaptainLEVI-XXX
Copy link

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

1 participant