fix: Security audit fixes for Autolisting.sol

[rroland10]

Summary

Comprehensive security audit and fixes for contracts/dex-core/Autolisting.sol, addressing 13 vulnerabilities across all four contracts in the file: AutoListingsRegistry, Dex223AutoListing, Dex223CoreAutoListing, and Dex223OwnableListing.

---

Vulnerabilities Found & Fixed

CRITICAL Severity

1. Missing Access Control on updateMe() — Dex223AutoListing & Dex223CoreAutoListing

• Risk: updateMe() was public with no access control, allowing any address to overwrite the contract URL and push arbitrary URL data to the registry. An attacker could redirect users to a phishing URL via the registry events.
• Fix: Added onlyOwner modifier to both functions.

2. Reentrancy in list() and listSingle() via External Calls

• Risk: list() performs state changes (checkListing updates listed_tokens, tokens, num_listed_tokens) before calling safeTransferFrom() which makes an external call to an arbitrary ERC-20 token. A malicious token could reenter list() during the transferFrom call, listing tokens without paying. registry.recordListing() inside checkListing is also an external call happening mid-execution.
• Fix: Added nonReentrant mutex modifier to list() and listSingle() in all contracts.

3. address(0) Token Bypass via isListed() / listed_tokens Collision

• Risk: isListed() returns listed_tokens[_token] != 0. Since token IDs start at 1, listed_tokens[address(0)] defaults to 0 (not listed). However, the old code blindly wrote listed_tokens[address(0)] = num_listed_tokens when a token had address(0) for one standard, polluting the mapping slot for address(0). Once set to any nonzero value, every future token with address(0) as one of its standards would appear already listed, causing checkListing to take the wrong branch and corrupt state.
• Fix: Added address(0) guards in checkListing() so we never write to listed_tokens[address(0)]. Introduced _isFullyListed() helper that correctly handles address(0) standards.

---

HIGH Severity

4. Double-Charge Bug: || Operator Causes Overcharging

• Risk: The condition !isListed(_token0_erc20) || !isListed(_token0_erc223) meant users were charged the listing fee even when the token was partially listed (one standard listed, the other not). The add-second-standard operation was not meant to cost an additional fee.
• Fix: Replaced with _isFullyListed() helper that returns true only when all non-zero standard addresses are already listed.

5. Excess ETH Not Refunded — Permanent User Fund Loss

• Risk: When paying with native ETH, if msg.value > toTransfer, the excess ETH was permanently trapped. The code explicitly stated // NOTE no return of excess payment. Especially problematic when price changes between TX submission and execution.
• Fix: Added automatic refund of msg.value - toTransfer via low-level call.

6. ETH Stuck When Paying with ERC-20 Token

• Risk: If a user mistakenly sent ETH while paying with an ERC-20 token (paymentToken != address(0)), the ETH was silently accepted and permanently stuck. Only the owner could retrieve it.
• Fix: Added require(msg.value == 0) in the ERC-20 branch and when no transfer is needed.

7. Dex223OwnableListing.updateMe() Allows Arbitrary Registry Replacement

• Risk: The old signature updateMe(string, string, address _registry) allowed the owner to point registry to any arbitrary address. A compromised owner could point it to a contract that silently fails, breaking event logging.
• Fix: Removed the _registry parameter. Registry is now immutable after construction.

8. listTokenByOwner() Does Not Update Internal State

• Risk: The function emitted events and called registry.recordListing() but never updated listed_tokens, tokens, or num_listed_tokens. Tokens listed via this function were invisible to isListed(), getToken(), and num_listed_tokens. Subsequent list() calls would re-list the same token, corrupting state.
• Fix: Now calls checkListing(_token20, _token223) which properly updates all internal state.

---

MEDIUM Severity

9. No Ownership Transfer Mechanism

• Risk: If the owner private key was compromised or lost, admin functions were permanently locked or compromised with no recovery path.
• Fix: Added transferOwnership(address _newOwner) with onlyOwner modifier, zero-address check, and OwnershipTransferred event to all three listing contracts.

10. extractTokens() Uses Vulnerable .transfer() for ETH

• Risk: payable(msg.sender).transfer(_amount) forwards only 2300 gas, which fails if the owner is a contract (e.g., Gnosis Safe multisig). This would permanently trap ETH.
• Fix: Replaced with low-level call which forwards all available gas.

---

LOW Severity

11. Typo: tokenUnanned in AutoListingsRegistry

• Risk: Function name tokenUnanned was a typo for tokenUnbanned. Affects ABI consistency and off-chain tooling.
• Fix: Renamed to tokenUnbanned().

12. Inconsistent onlyOwner Pattern

• Risk: Some functions used inline require(msg.sender == owner) while others used the modifier. Inconsistent patterns increase the risk of missing access control on new functions.
• Fix: Added onlyOwner modifier to all three listing contracts and used it consistently.

13. Missing Error Messages in require Statements

• Risk: Empty require statements provide no debugging information when transactions revert.
• Fix: Added descriptive error messages to all require statements.

---

Additional Changes

• hardhat.config.ts: Added compiler overrides for Revenue_old.sol and RevenueV1.sol (pragma ^0.8.13 mapped to compiler 0.8.19) to fix pre-existing compilation config errors unrelated to this audit.

Compilation

• Autolisting.sol compiles with zero errors and zero warnings.
• Pre-existing compilation errors in unrelated test/mock files (MockTimeDex223Pool.sol, Dex223PoolLib.sol) remain unchanged.

Test Plan

• Verify updateMe() reverts when called by non-owner
• Verify list() correctly refunds excess ETH payment
• Verify list() reverts when sending ETH with ERC-20 payment token
• Verify list() is protected against reentrancy via malicious token
• Verify checkListing() does not write to listed_tokens[address(0)]
• Verify _isFullyListed() returns correct results for partial listings
• Verify listTokenByOwner() updates listed_tokens, tokens, num_listed_tokens
• Verify extractTokens() works with multisig/contract owners
• Verify transferOwnership() correctly transfers and emits event
• Verify setPaymentPrice() uses onlyOwner modifier
• Regression test: existing listing flows still work end-to-end