auction integration

[KillariDev]

• use the new auction contract with other contracts
• remove old auction contract

[github-actions]

🛡️ Defender Against the Dark Arts

4 issues found

Invalid Solidity version pragma

📍 solidity/contracts/peripherals/interfaces/IDualCapBatchAuction.sol:2

Line 2 specifies pragma solidity 0.8.33;, but no such official Solidity compiler version exists (as of current releases). This is a typographical error that will prevent compilation and may indicate tampering or a mistaken version reference. It could be an attempt to exploit non-existent or future compiler behaviors.

Unverified New Dependency Introduced

📍 solidity/contracts/peripherals/interfaces/ISecurityPool.sol:6

The code diff replaces a dependency on Auction with DualCapBatchAuction without any context or verification of the new contract's integrity. This introduces an unverified new dependency that could be part of a supply chain compromise or contain hidden malicious logic. The change appears in a security-critical interface that deploys child security pools, making the trustworthiness of the auction contract paramount.

Critical Interface Function Signature Altered Without Justification

📍 solidity/contracts/peripherals/interfaces/ISecurityPool.sol:104

The factory function deployChildSecurityPool changes its return type from Auction to DualCapBatchAuction in the interface. This is a breaking change for all consumers of this interface and could be a deliberate attempt to redirect deployments to an unverified contract. The modification lacks any accompanying documentation or migration path, which is highly suspicious for a security-sensitive contract factory.

Debugger statement left in production error handling code

📍 solidity/ts/compile.ts:290

The addition of a 'debugger' statement in the catch block of compileContracts() can cause the process to pause execution and wait for a debugger to attach. In production environments, this may lead to denial of service if no debugger is present, or could facilitate unauthorized debugging and potential inspection or manipulation of process state by attackers who can trigger errors. This is a significant security concern as it deviates from standard error handling practices and introduces a latent debugging backdoor-like behavior.

[github-actions]

🏛️ Architect

3 issues found

Concrete auction type stored in high-level module

📍 solidity/contracts/peripherals/SecurityPoolForker.sol:19

The ForkData struct stores truthAuction as the concrete type DualCapBatchAuction instead of the interface IDualCapBatchAuction. This violates the Dependency Inversion Principle, creating tight coupling to a specific implementation and hindering flexibility for future changes or alternative auction types.

Leaky abstraction in public API due to auction-specific parameter

📍 solidity/contracts/peripherals/SecurityPoolForker.sol:231

The claimAuctionProceeds function requires IDualCapBatchAuction.TickIndex[] as a parameter, exposing the internal tick-based structure of the auction implementation. This leaks abstraction details into the ISecurityPoolForker interface, increasing coupling and forcing external callers to understand auction-specific concepts, which violates information hiding and complicates maintenance.

Event and function signatures use concrete types leading to ABI instability

📍 solidity/contracts/peripherals/factories/SecurityPoolFactory.sol:28

Replacing the Auction type with DualCapBatchAuction in the SecurityPoolFactory's DeploySecurityPool event and deployChildSecurityPool return type changes the ABI signatures. The event's keccak signature changes (different parameter types), breaking any off-chain consumers that rely on the previous event signature. The function return type change also breaks callers that expect the Auction interface because the concrete type is different, potentially causing runtime errors if the new contract does not match the old interface. To maintain stability, use an abstract interface (e.g., IAuction) or a generic type like address for external APIs and events.

[github-actions]

🔧 Expensive Linter

1 issue found

Inconsistent blank lines around function definitions

📍 solidity/contracts/peripherals/SecurityPoolForker.sol:258

The newly added receive function is directly attached to the preceding function getMarketOutcome without the required separating blank line. Additionally, there is an extra blank line after receive before the contract's closing brace, whereas the established style does not place a blank line after the final function. Please insert a blank line after the closing brace of getMarketOutcome (line 258) and remove the blank line after receive (currently line 260).

[github-actions]

🧠 Wise Man

6 issues found

Remove unused function _wouldClear

📍 solidity/contracts/peripherals/DualCapBatchAuction.sol:209

The function _wouldClear is defined but never called within the contract. Unused code adds unnecessary complexity, increases maintenance burden, and can confuse future readers. Removing it aligns with clean code principles and reduces the attack surface.

Invalid data location for external function parameter

📍 solidity/contracts/peripherals/interfaces/ISecurityPoolForker.sol:15

The parameter tickIndices in the external function claimAuctionProceeds is declared with memory data location. For external functions in Solidity, complex types like arrays must use calldata (or omit the data location, which defaults to calldata for external functions). Using memory will cause a compilation error.

CompilationError catch block does not log stack trace

📍 solidity/ts/compile.ts:284

In the catch block, CompilationError is logged via error.toString(), which returns a custom string without the stack trace. For other errors, the error object is logged directly, which includes the stack. This inconsistency means that when a CompilationError occurs, the stack trace is lost, making debugging significantly harder.

Incorrect tolerance values in approximatelyEqual assertions

📍 solidity/ts/tests/peripherals.test.ts:400

The tolerance argument in approximatelyEqual must be a small absolute delta appropriate for the values being compared. Several assertions mistakenly use the expected value itself, or excessively large tolerances (e.g., openInterestAmount * 1000n, or constants many orders of magnitude larger than the expected). This makes the assertions effectively always pass, undermining test reliability. The correct tolerances should be small constants like 10n for exact comparisons, 10000n for REP amounts, or 10n**15n for ETH balances, as originally used before these changes.

Magic number used for price precision instead of defined constant

📍 solidity/ts/tests/peripherals.test.ts:395

The REP calculation uses a hardcoded magic number 1_000_000_000_000_000_000n (10^18) as the price precision multiplier, despite a constant PRICE_PRECISION being defined at line 25. This magic number should be replaced with the named constant to improve readability and maintainability, ensuring a single source of truth for this important value.

Misleading variable name 'securityPoolSaltWithMsgSender'

📍 solidity/ts/testsuite/simulator/utils/contracts/deployPeripherals.ts:195

The variable 'securityPoolSaltWithMsgSender' suggests that the salt incorporates the msg.sender, but the actual computation only hashes the securityPoolFactory address and the securityPoolSalt. This misnomer can lead to confusion for future maintainers, as the salt is used for deploying the priceOracleManagerAndOperatorQueuer contract and does not involve msg.sender at all. Renaming the variable to something like 'oracleManagerSalt' would accurately reflect its purpose and improve code clarity.

[github-actions]

🐛 Bug Hunter

7 issues found

Invalid destructuring assignment in refundLosingBids

📍 solidity/contracts/peripherals/DualCapBatchAuction.sol:164

The destructuring assignment for computeClearing return values on line 164 is missing identifiers for the third and fourth values, causing a compilation error. It must provide identifiers for all four return values.

Loss of funds for zero clearing price in withdrawBids

📍 solidity/contracts/peripherals/DualCapBatchAuction.sol:122

For bids with tick greater than clearingTick, if clearingPriceLocal is zero, the ETH is not refunded and no REP is issued, resulting in a loss of funds for the bidder. The else case to refund ETH is missing.

Tree state inconsistency after withdrawBids

📍 solidity/contracts/peripherals/DualCapBatchAuction.sol:152

After processing a bid in withdrawBids, the tree node totals are not decreased, leaving stale data. This causes computeClearing to include withdrawn bids and return incorrect results, potentially misleading off-chain systems.

Inconsistent parameter in refundLosingBids interface

📍 solidity/contracts/peripherals/interfaces/IDualCapBatchAuction.sol:56

The function refundLosingBids is declared without a bidder address parameter, but the corresponding event RefundLosingBids requires a bidder address. This mismatch prevents the implementing contract from correctly emitting the event, as the bidder cannot be determined from the function parameters alone, leading to potential incorrect or missing bidder data in the event.

Invalid data location 'memory' for external function array parameter

📍 solidity/contracts/peripherals/interfaces/ISecurityPoolForker.sol:15

In Solidity, external function parameters of array or struct type must use 'calldata' data location. Specifying 'memory' for such parameters is invalid and will cause a compilation error, preventing the code from working. The parameter 'tickIndices' is an array (IDualCapBatchAuction.TickIndex[]) and is incorrectly marked as 'memory' in an external function.

Incorrect escape handling in CompilationError.toString()

📍 solidity/ts/compile.ts:71

The toString() method uses an unescape function that replaces \n and \t with newline and tab characters. This is incorrect because: (1) The error messages from solc are already properly unescaped by JSON.parse and contain actual newline/tab characters. (2) The regex /\n/g matches a literal backslash followed by 'n', which would incorrectly corrupt any legitimate backslash-n sequences in error messages (e.g., file paths like C:\path\to\file). This will cause data corruption in error output.

Test assertions use excessively large tolerance values in approximatelyEqual calls

📍 solidity/ts/tests/peripherals.test.ts:399

In the 'two security pools with disagreement' test, multiple approximatelyEqual calls have tolerance parameters set to values that are orders of magnitude too large (e.g., the expected value itself, openInterestAmount * 1000n, or repBalanceInGenesisPool). This makes the assertions ineffective, as they will pass even with huge deviations from expected values, potentially allowing bugs to go undetected. The tolerances should be small absolute numbers consistent with financial precision: use 10n**15n for ETH and collateral amounts, and 1_000n for REP amounts, as seen in other correct assertions within the same test (e.g., lines 380, 385, 395, 435, 469).

[github-actions]

🔒 Security Reviewer

3 issues found

Missing check for clearing condition in finalize

📍 solidity/contracts/peripherals/DualCapBatchAuction.sol:90

The finalize function does not verify the priceFound flag returned by computeClearing. If no valid clearing is found (i.e., the auction did not meet the ETH raise cap or REP sale target), the contract still finalizes, setting an arbitrary clearingTick and sending accumulatedEth to the owner. This leads to an incorrect distribution: bids with ticks above the arbitrary clearing tick are treated as winning but receive no REP, and the REP/ETH allocation becomes inconsistent. An attacker can manipulate bids (e.g., by placing bids only at extreme zero-price ticks) to cause priceFound to be false, causing loss of funds for other bidders when the owner finalizes.

Unbounded tree size enables gas exhaustion DoS

📍 solidity/contracts/peripherals/DualCapBatchAuction.sol

The contract stores bids in a binary search tree, and _compute performs a full in-order traversal to determine the clearing price. There is no limit on the number of distinct ticks (nodes) in the tree. An attacker can place many bids at different ticks (even at the minimum size) to create a large tree. The gas cost of _compute is linear in the number of nodes; with a sufficiently large tree (e.g., > few thousand nodes) the required gas can exceed the block gas limit. This causes finalize and refundLosingBids (which also call computeClearing) to revert, locking the auction and preventing participants from withdrawing funds. The lack of a node limit makes the contract vulnerable to a denial-of-service attack.

Missing Access Control on claimAuctionProceeds

📍 solidity/contracts/peripherals/SecurityPoolForker.sol:231

The claimAuctionProceeds function allows any caller to specify any vault address and withdraw REP proceeds from the truth auction for that vault. This enables an attacker to claim the auction proceeds of any vault, either by withdrawing the full amount (if they know all tick indices) or by partially withdrawing and preventing the legitimate owner from claiming the remainder, leading to theft of funds or denial of service for vault owners.