Description
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
- Use of
transfer()
Instead ofcall()
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