This PR introduces the CLAWNFA contract

[tomzlabs]

Key Changes

• Added [contracts/claw/CLAWNFA.sol]:
  • Implements the IBAP578 interface for full agent lifecycle management (state, metadata, execution).
  • Uses a minimal Ownable implementation.
• Open Source Improvements:
  • Removed hardcoded addresses (specifically REQUIRED_TOKEN and signerAddress).
  • Made critical parameters (REQUIRED_TOKEN, MAX_SUPPLY) configurable via the constructor.
  • Removed unused variables (e.g., nonces, signerAddress) to streamline the contract.
• Security Check:
  • Verified no sensitive addresses or keys are hardcoded.

Verification

• Contract compiles successfully.
• Constructor arguments allow flexible deployment for different networks/tokens.

[tomzlabs]

update

[christelbuchanan]

Thank you @tomzlabs, we will review quickly.
@iflp and @snowmanxm for your attention

[highsoft-85]

@tomzlabs , please resolve the failed actions

[saiboyizhan]

Hi @tomzlabs, great work on the CLAWNFA contract! I took a closer look at the code and wanted to share some security observations that might help strengthen the implementation before it gets merged. Hope this is helpful!

---

Security Review — CLAWNFA.sol

Summary

Severity	Count
Critical	2
High	3
Medium	4
Low	6
Info	5

---

Critical

C-1: delegatecall in executeAction() allows storage corruption and fund theft

(bool ok, bytes memory result) = s.logicAddress.delegatecall(data);

delegatecall executes the logic contract's code in the NFA contract's storage context. This means any whitelisted logic contract has full read/write access to ALL storage variables (_owner, _owners, _balances, signerAddress, etc.) and the contract's ETH balance. Even a well-intentioned logic contract with a subtle bug could accidentally overwrite critical storage slots.

Suggestion: Replace with a regular call, or use ERC-6551 token-bound accounts to isolate each agent's storage:

(bool ok, bytes memory result) = s.logicAddress.call(data);

---

C-2: ecrecover signature malleability — missing s value check

The _recover() function uses raw ecrecover without checking the s value range. For every valid ECDSA signature (v, r, s), there exists a second valid signature (v', r, n-s). Combined with the fact that ecrecover returns address(0) on invalid inputs — if signerAddress were ever set to address(0), any invalid signature would pass verification.

Suggestion: Add OpenZeppelin's standard s value check:

require(uint256(s) <= 0x7FFFFFFFFFFFFFFFFFFFFFFFFFFFFFFF5D576E7357A4501DDFE92F46681B20A0,
        "Invalid signature 's' value");
require(v == 27 || v == 28, "Invalid signature 'v' value");
address recovered = ecrecover(digest, v, r, s);
require(recovered != address(0), "Invalid signature");

Or consider using OpenZeppelin's ECDSA.recover() which handles all of this.

---

High

H-1: Funded ETH is permanently locked for token holders

fundAgent() tracks per-token balances via _states[tokenId].balance, but there is no withdrawFromAgent() function for token owners to withdraw their funds. The only withdrawal mechanism is withdraw() which is onlyOwner and sweeps the entire contract balance regardless of per-token accounting. This means:

• Users fund their agents believing they can withdraw later
• Only the contract owner can ever withdraw, and it takes everything

Suggestion: Add a per-token withdrawal function:

function withdrawFromAgent(uint256 tokenId, uint256 amount) external nonReentrant {
    require(ownerOf(tokenId) == msg.sender, "NFA: not owner");
    require(_states[tokenId].balance >= amount, "NFA: insufficient");
    _states[tokenId].balance -= amount;
    (bool success, ) = msg.sender.call{value: amount}("");
    require(success, "NFA: withdraw failed");
}

H-2: setSignerAddress allows zero address

function setSignerAddress(address newSigner) external onlyOwner {
    signerAddress = newSigner;  // No zero-address check, no event
}

Setting signerAddress = address(0) + C-2's ecrecover behavior = anyone can mint with invalid signatures. Also missing an event for off-chain monitoring.

---

H-3: Terminated agents can be revived via unpause()

function unpause(uint256 tokenId) external override {
    require(ownerOf(tokenId) == msg.sender, "NFA: not owner");
    _states[tokenId].status = Status.Active;  // No check for Terminated!
}

A terminated agent should never be reactivatable. Also, fundAgent() has no check against terminated agents — ETH sent to terminated agents is locked forever.

Suggestion: Add status validation:

function unpause(uint256 tokenId) external override {
    require(ownerOf(tokenId) == msg.sender, "NFA: not owner");
    require(_states[tokenId].status == Status.Paused, "NFA: not paused");
    _states[tokenId].status = Status.Active;
}

---

Medium

M-1: Events emit address(this) instead of tokenId

All events use address(this) as the agent identifier:

emit AgentFunded(address(this), msg.sender, msg.value);
emit StatusChanged(address(this), Status.Paused);

Since all tokens share the same contract, address(this) is identical for every event — making it impossible for indexers/subgraphs to track which specific agent was affected.

M-2: _transfer() doesn't update _states[tokenId].owner for terminated agents

When transferring a terminated agent NFT, _owners[tokenId] updates but _states[tokenId].owner does not, creating an owner inconsistency.

M-3: balanceOf(address(0)) should revert per ERC-721 spec

Current implementation returns 0 for zero address instead of reverting.

M-4: No totalSupply() function

_nextTokenId is private with no getter. Many marketplace integrations expect totalSupply().

---

Low

• L-1: setAllowedLogicContract() emits no event (critical governance action given delegatecall)
• L-2: setBaseURI() emits no event
• L-3: No renounceOwnership() in custom Ownable
• L-4: Single-step ownership transfer (consider Ownable2Step)
• L-5: name and symbol are mutable storage (could be corrupted via delegatecall)
• L-6: DOMAIN_SEPARATOR computed once at deploy — no chain fork protection

Informational

• I-1: receive() accepts ETH without tracking (not attributed to any agent)
• I-2: Consider custom errors instead of revert strings for gas savings
• I-3: Token IDs start at 0 (some integrations expect 1)
• I-4: Consider exact pragma solidity 0.8.20 for production
• I-5: No reentrancy guard on withdraw() function

---

Overall

The most impactful change would be replacing delegatecall with call in executeAction() — this single change eliminates C-1, reduces L-5 risk, and is a straightforward fix. The second priority would be adding OpenZeppelin's ECDSA library for signature verification (fixes C-2).

Happy to discuss any of these further. Nice foundation — looking forward to seeing this evolve! 🤝

[saiboyizhan]

Hi @tomzlabs, great response on the security fixes! The critical and high-severity items are well addressed. A few remaining observations:

---

Remaining Security Items

1. Single-step ownership transfer (Low)

function transferOwnership(address newOwner) external onlyOwner {
    require(newOwner != address(0), "Owner: zero address");
    emit OwnershipTransferred(_owner, newOwner);
    _owner = newOwner;
}

If newOwner is entered incorrectly, ownership is permanently lost. Consider a two-step pattern (transferOwnership + acceptOwnership) similar to OpenZeppelin's Ownable2Step.

2. DOMAIN_SEPARATOR is immutable — no chain fork protection (Low)

DOMAIN_SEPARATOR is computed once in the constructor. If the chain forks, EIP-712 signatures would be valid on both chains, enabling cross-chain replay of mint signatures. Consider computing it dynamically with chainid() comparison.

3. receive() accepts ETH without attribution (Info)

The current receive() accepts and logs unattributed ETH. Note that the official BAP578.sol reverts on direct transfers:

// BAP578.sol
receive() external payable {
    revert("Use fundAgent() instead");
}

This is a design choice, but worth aligning with the standard if full compliance is the goal.

---

Structural Deviations from BAP578.sol

For the maintainers' reference — the following are not bugs, but architectural differences from the official BAP578.sol reference implementation that may affect merge decisions:

Aspect	BAP578.sol Standard	CLAWNFA.sol
Agent status	bool active	enum Status { Active, Paused, Terminated }
Upgradeability	UUPS (UUPSUpgradeable)	Non-upgradeable
ReentrancyGuard	OpenZeppelin's ReentrancyGuardUpgradeable	Custom _locked boolean
Contract-level pause	whenNotPaused modifier on createAgent/fundAgent	Not present
Access control	onlyTokenOwner(tokenId) modifier	Inline require(ownerOf(tokenId) == msg.sender)
Event signatures	AgentFunded(uint256 indexed tokenId, uint256 amount)	AgentFunded(address indexed agent, address indexed funder, uint256 amount)
Burn function	_burn() override requiring balance == 0	Not implemented
Mint mechanism	0.01 ETH fee + free mint quota	ERC-20 token gate + EIP-712 signature
receive()	Reverts	Accepts + emits event

These are design choices — CLAWNFA adds features like EIP-712 minting, logic contract governance, and three-state lifecycle that BAP578.sol doesn't have. However, certain deviations (especially event signatures and missing whenNotPaused) could affect ecosystem tooling compatibility.

---

Overall the security posture is much stronger after the latest commit. The above items are lower priority but worth considering for long-term maintainability. 👍

[christelbuchanan]

Thanks @saiboyizhan for jumping in much appreciated
@tomzlabs waiting for your changes!