BattleChain Confidence Pools
Cyfrin
FoundrySolidityLayer 2
7.25 ETH
View all submissions
Previous Next
Submission Details
Impact: low
Likelihood: high

Redundant validation checks and assignments waste gas

Author Revealed upon completion

Description

  • Under normal behavior, the ConfidencePoolFactory.createPool() function handles the deployment and initialization of new Confidence Pools. Because the factory is the only authorized caller to initialize(), any input validation performed in the factory does not need to be repeated in the pool's initialization function. Additionally, certain state variables default to their zero or uninitialized values in Solidity, meaning explicit assignments to these defaults are unnecessary.

  • The ConfidencePool.initialize() and ConfidencePool.stake() functions contain multiple redundant checks and assignments. Specifically:

    1. agreement_, stakeToken_, recoveryAddress_, expiry_, and isAgreementValid are all strictly validated in ConfidencePoolFactory.createPool() before initialize() is even called.

    2. safeHarborRegistry_, outcomeModerator_, and owner_ are passed directly from trusted factory state or msg.sender and cannot be address(0).

    3. outcome = PoolStates.Outcome.UNRESOLVED is redundant because UNRESOLVED is the first enum variant (value 0), which is the default value for an uninitialized storage variable.

    4. In stake(), if (amount == 0) is redundant because minStake is already strictly required to be > 0 during initialization, and stake() immediately checks if (amount < minStake). Any amount == 0 will naturally fail the minStake check.

function initialize(...) external initializer {
// @> All of these checks are entirely redundant and already enforced by the factory
if (agreement_ == address(0)) revert ZeroAddress(); //@audit, already checked in factory
if (stakeToken_ == address(0)) revert ZeroAddress(); //@audit, already checked in factory
if (safeHarborRegistry_ == address(0)) revert ZeroAddress(); //@audit safeHarborRegistry_ can never be zero
if (outcomeModerator_ == address(0)) revert ZeroAddress(); //audit, outcomeModerator_ can never be zero
if (owner_ == address(0)) revert ZeroAddress(); //@audit owner can never be zero
if (recoveryAddress_ == address(0)) revert InvalidRecoveryAddress(); //@audit, already checked in factory
if (expiry_ < block.timestamp + _MIN_EXPIRY_LEAD) revert ExpiryTooSoon(); //@audit, ghis has been checked for in factory already
// ...
// @> Redundant external call and registry check
if (!IBattleChainSafeHarborRegistry(safeHarborRegistry_).isAgreementValid(agreement_)) {
revert InvalidAgreement();
} //@audit , this has been checked for in the factory
​
// ...
​
// @> Redundant assignment to default value
outcome = PoolStates.Outcome.UNRESOLVED; //@audit, redundant, PoolStates is unresolved by default
}
​
function stake(uint256 amount) external nonReentrant whenPoolNotPaused {
// @> Redundant check because amount < minStake is checked immediately after
if (amount == 0) revert InvalidAmount(); //@audit, this check is unecessary since minStake has been checjed against 0, and amount is checked againt minStake
if (amount < minStake) revert BelowMinStake();

Risk

Likelihood: High

  • These redundant checks execute on every pool initialization and every stake action.

Impact: Low

  • Wastes deployment and runtime gas with no security benefit.

Proof of Concept

The redundancy is self-evident by cross-referencing ConfidencePoolFactory.createPool() and ConfidencePool.initialize().

Recommended Mitigation

Remove the redundant checks and assignments to optimize gas consumption.

function initialize(...) external initializer {
- if (agreement_ == address(0)) revert ZeroAddress();
- if (stakeToken_ == address(0)) revert ZeroAddress();
- if (safeHarborRegistry_ == address(0)) revert ZeroAddress();
- if (outcomeModerator_ == address(0)) revert ZeroAddress();
- if (owner_ == address(0)) revert ZeroAddress();
- if (recoveryAddress_ == address(0)) revert InvalidRecoveryAddress();
- if (expiry_ < block.timestamp + _MIN_EXPIRY_LEAD) revert ExpiryTooSoon();
if (expiry_ > type(uint32).max) revert ExpiryTooFar();
if (minStake_ == 0) revert InvalidAmount();
- if (!IBattleChainSafeHarborRegistry(safeHarborRegistry_).isAgreementValid(agreement_)) {
- revert InvalidAgreement();
- }
​
agreement = agreement_;
stakeToken = IERC20(stakeToken_);
safeHarborRegistry = IBattleChainSafeHarborRegistry(safeHarborRegistry_);
outcomeModerator = outcomeModerator_;
expiry = uint32(expiry_);
minStake = minStake_;
recoveryAddress = recoveryAddress_;
- outcome = PoolStates.Outcome.UNRESOLVED;
}
​
function stake(uint256 amount) external nonReentrant whenPoolNotPaused {
- if (amount == 0) revert InvalidAmount();
if (amount < minStake) revert BelowMinStake();
}

Missing Interface Documentation

In addition to the redundant checks, the codebase includes @inheritdoc tags for functions that have no corresponding NatSpec in the interface.

  • sweepUnclaimedBonus() is annotated with /// @inheritdoc IConfidencePool in ConfidencePool.sol, but the docs don't exist in IConfidencePool.

  • sweepUnclaimedCorrupted() is also annotated with /// @inheritdoc IConfidencePool, but the docs are missing in the interface.

// @> Missing corresponding NatSpec in IConfidencePool.sol
/// @inheritdoc IConfidencePool
function sweepUnclaimedBonus() external onlyOwner { ... }
​
// @> Missing corresponding NatSpec in IConfidencePool.sol
/// @inheritdoc IConfidencePool
function sweepUnclaimedCorrupted() external onlyOwner { ... }

Recommendation: Add the missing NatSpec documentation for both functions directly to IConfidencePool.sol.

Support

FAQs

Can't find an answer? Chat with us on Discord, Twitter or Linkedin.

Give us feedback!