BetterBank Custom Contracts - BetterBank

The update function in the LPOracle contract enforces a maximum cap on price growth by comparing the new LP price with the last recorded price multiplied by the cap:

uint256 sqrt0 = Math.sqrt(uint256(usd0Average._x));
uint256 sqrt1 = Math.sqrt(uint256(usd1Average._x));
uint256 kx = uint256(kAverage._x);
uint256 part = kx * 2 * sqrt0 / 2 ** 56;
uint256 lpQ112 = part * sqrt1 / 2 ** 56;
uint256 newLpPrice = lpQ112 * 1e18 >> 112;

if (lastLpPrice == 0 || lpPriceCap == 0) {
    lastLpPrice = newLpPrice;
} else {
    uint256 maxAllowed = lastLpPrice * lpPriceCap / 1e18;
    if (newLpPrice <= maxAllowed) {
        lastLpPrice = newLpPrice;
    }
}

If the new LP price exceeds this threshold, the oracle ignores the update and keeps the old value. This can cause the oracle to remain stuck at a stale low price while the actual market LP price is much higher, enabling arbitrage against contracts that rely on the oracle.

Example:

• Current oracle price = $100.

• Cap allows at most a 2x increase → maxAllowed = $200.

• Real LP price moves to $210 (legitimately or via manipulation).

• Since $210 > $200, the contract rejects the update → oracle remains stuck at $100.

It is recommended to clamp the new LP price to the maximum allowed value instead of ignoring the update, ensuring the oracle always progresses and does not freeze at stale prices.

SOLVED: The Better Bank team solved the issue in the specified commit id.

In the Staking contract, the allocateSeigniorage function computes rewardPerShare using the live totalSupply at the time of execution:

function allocateSeigniorage(uint256 amount) external nonReentrant whenNotPaused {
    require(msg.sender == owner() || msg.sender == treasuryOperator, "Not authorized");
    require(amount > 0, "Grove: Cannot allocate 0");
    require(totalSupply() > 0, "Grove: Cannot allocate when totalSupply is 0");

    // Calculate new reward per share
    uint256 prevRPS = getLatestSnapshot().rewardPerShare;
    uint256 nextRPS = prevRPS + ((amount * 1e18) / totalSupply());

    // Store new snapshot
    historyEnd += 1;
    groveHistory[historyEnd] = GroveSnapshot({
        time: block.timestamp,
        rewardReceived: amount,
        rewardPerShare: nextRPS
    });

    // Enforce maximum snapshot history
    if (historyEnd - historyStart + 1 > MAX_HISTORY) {
        delete groveHistory[historyStart];
        historyStart += 1;
    }

    // Transfer rewards from sender to contract
    favor.safeTransferFrom(msg.sender, address(this), amount);

    emit RewardAdded(msg.sender, amount);
}

Because the total supply is read at allocation time, any newly staked Esteem is immediately included in the reward calculation. This allows users who stake right before allocateSeigniorage is called to capture a significant portion of the distribution, diluting existing stakers. Such strategic staking does not break the contract’s rules, but it creates an imbalance in how rewards are allocated and may incentivize short-term movements rather than stable participation.

It is recommended to enforce a minimum stake lock period lasting at least until the end of the next epoch, preventing last-minute staking solely to capture current rewards.

SOLVED: The Better Bank team solved the issue in the specified commit id.

In the Oracle contract, the consult function is intended to return the output value for a given _amountIn. For token0 and token1, the result is correctly scaled by the input amount:

function consult(address _token, uint256 _amountIn)
    external view
    returns (uint256 amountOut)
{
        if (_token == token0) {
            amountOut = uint256(price0Average.mul(_amountIn).decode144());
        } else if (_token == token1) {
            amountOut = uint256(price1Average.mul(_amountIn).decode144());
        } else if (_token == address(pair)) {
            amountOut = lastLpPrice;
        } else {
            revert("Oracle: INVALID_TOKEN");
        }

}

However, in the LP token branch, the function ignores _amountIn and always returns lastLpPrice. This causes the output to remain constant regardless of the input, breaking consistency with the intended pricing logic.

It is recommended to scale the LP token price by multiplying lastLpPrice with _amountIn and normalizing by 1e18.

SOLVED: The Better Bank team solved the issue in the specified commit id.

In the MintRedeemer.updateEsteemRate() function, the Esteem price is increased by a fixed dailyRateIncrease only when explicitly called by an approved user. If no one calls this function for multiple epochs (e.g., several days), the rate remains stale and underpriced. When users mint or redeem during this period, they transact at an outdated esteemRate, causing systemic under-valuation.

In the MintRedeemer contract, the updateEsteemRate function increases the Esteem rate by a fixed dailyRateIncrease only when explicitly called by an approved user:

function updateEsteemRate() external {
    require(isApprovedUser[msg.sender], "Not approved user");
    require(block.timestamp >= startTime, "not started");
    require(block.timestamp >= nextEpochTime(), "Updater epoch not passed");
    epoch += 1;
    esteemRate += dailyRateIncrease;
    emit RateUpdated(esteemRate);
}

If no one triggers this function for several epochs, the esteemRate remains stale and underpriced. When users mint or redeem during this time, they interact with an outdated rate, leading to systemic undervaluation.

Example:

• Initial esteemRate = 21.00

• dailyRateIncrease = 0.25

• After 3 days, the correct rate should be 21.75

• If no update call is made, the rate remains 21.00 and all mints/redeems are undervalued

It is recommended to allow anyone to call the update function and ensure the rate is refreshed automatically during mint and redeem.

SOLVED: The Better Bank team solved the issue in the specified commit id. MintRedeemer no longer uses epochs, now uses block.timestamp price updates.

The Zapper contract calls Uniswap's swapExactTokensForTokensSupportingFeeOnTransferTokens and addLiquidity functions with parameters amountOutMin = 0, amountAMin = 0 and amountBMin = 0. It also applies very short deadlines such as block.timestamp + 50. This configuration leaves swaps and liquidity additions fully exposed to front-running and sandwich attacks, allowing execution at highly unfavorable prices.

It is recommended to enforce non-zero minimum amounts and user-provided deadlines to ensure swaps and liquidity additions fail if prices move beyond an acceptable tolerance.

SOLVED: The Better Bank team solved the issue in the specified commit id.

The contracts initialize ownership through the single-step Ownable pattern, for example by calling Ownable(_owner) in the constructor. In this model, ownership is either set directly in the constructor or transferred in one transaction with transferOwnership(newOwner), which the new owner implicitly accepts. Because there is no explicit acceptance, a mistaken address or malicious input can cause irreversible loss of control, as there is no handshake mechanism to confirm ownership transfer.

It is recommended to replace the single-step Ownable implementation with a two-step process such as OpenZeppelin's Ownable2Step, requiring the new owner to explicitly accept ownership before the transfer is finalized.

SOLVED: The Better Bank team solved the issue in the specified commit id.

In the Favor contract's _update function, the logic checks whether either the from or to address is tax-exempt. If either is exempt, no tax is applied. This creates a loophole where funds can bypass taxation through an intermediate tax-exempt address:

function _update(address _from, address _to, uint256 _value) internal override {
        bool destinationIsContract = _to.code.length != 0;


        if (_isTaxExempt(_to) || _isTaxExempt(_from)) {
            super._update(_from, _to, _value);
            return;
 }

Example:

• Address A (non-exempt) wants to transfer tokens to Contract C (non-exempt).

• Instead of sending directly, A sends tokens to Contract B (tax-exempt).

• B then forwards the tokens to C.

• The effective flow of funds is from A to C, but since B is tax-exempt, the transfer from B to C avoids taxation.

This undermines the intended tax logic and could be exploited to systematically bypass the tax mechanism.

It is recommended to modify the _update logic to ensure that tax is enforced if both sender and receiver are non-exempt, regardless of intermediate transfers.

SOLVED: The Better Bank team solved the issue in the specified commit id. The Favor token is now fully gated. EOA transfers are allowed, and transfers only to contracts we set as tax exempt i.e., the LPZapper and Staking.

In the Staking contract, most core functions such as deposit and withdraw are protected by the whenNotPaused modifier to ensure the protocol can be halted during emergencies or maintenance.

However, the claimReward function does not include this modifier, allowing users to continue claiming rewards even while the contract is paused:

function claimReward() public updateReward(msg.sender) {
        uint256 reward = grovers[msg.sender].rewardEarned;
        if (reward > 0) {
            grovers[msg.sender].rewardEarned = 0;
            favor.safeTransfer(msg.sender, reward);
            emit RewardPaid(msg.sender, reward);
        }
 }

This issue breaks the intended pause mechanism and reduces the effectiveness of emergency controls.

It is recommended to add the whenNotPaused modifier to the claimReward function to align it with other core functions.

SOLVED: The Better Bank team solved the issue in the specified commit id. whenNotPaused was added to claimReward.

The epoch counter is initialized differently across contracts. In FavorTreasury, the starting epoch is set to 0, while in MintRedeemer it is set to 1. This inconsistency can create confusion for integrators and increase the risk of logic errors when comparing or synchronizing epoch-based calculations between contracts.

It is recommended to standardize the initialization of epoch counters across all contracts that rely on epoch-based logic to ensure consistent behavior and prevent discrepancies.

SOLVED: The Better Bank team solved the issue in the specified commit id. MintRedeemer no longer uses epochs, now uses block.timestamp price updates.

In the FavorTreasury contract, the removeExcludedAddress function searches the excludedAddresses array with a for loop to locate and remove an entry:

function removeExcludedAddress(address _address) external onlyOwner {
        require(excludedFromTotalSupply[_address], "Address not excluded");
        
        excludedFromTotalSupply[_address] = false;
        
        // Remove from array
        uint256 n = excludedAddresses.length;
        for (uint256 i = 0; i < n; i++) {
            if (excludedAddresses[i] == _address) {
                excludedAddresses[i] = excludedAddresses[n - 1];
                excludedAddresses.pop();
                break;
            }
        }
        
        emit ExcludedAddressRemoved(_address);
}

As the array grows, the gas cost of iterating increases linearly, and for sufficiently large arrays the function may run out of gas and never succeed. This makes it possible for excluded addresses to remain permanently unremovable once the list becomes too large.

It is recommended to enforce a hard limit on the number of excluded addresses added so that removal operations remain feasible within gas limits.

SOLVED: The Better Bank team solved the issue in the specified commit id. It was added MAX_EXCLUDED constant and check.

In the Zapper contract, the addFavor function allows the owner to register a Favor token with its LP pair and base token. However, the function does not validate whether a Favor token, base token, or LP has already been registered:

function addFavor(
    // favor token
    address _favor,
    // lp
    address _lp,
    // base token
    address _token
    ) external onlyOwner {
        require(_favor != address(0), "Invalid address");
        require(_lp != address(0), "Invalid address");
        require(_token != address(0), "Invalid address");
        favorToToken[_favor] = _token;
        favorToLp[_favor] = _lp;
        tokenToFavor[_token] = _favor;
        emit ActiveFavorTokenSet(_favor, _lp, _token);
 }

As a result, calling addFavor multiple times with different values can overwrite existing mappings in favorToToken, favorToLp, and tokenToFavor, leading to inconsistent state and potential misconfiguration.

It is recommended to add explicit checks that prevent registering a Favor, LP, or base token more than once, reverting if the mapping already exists.

SOLVED: The Better Bank team solved the issue in the specified commit id.

Several state variables across the contracts are assigned only once in the constructor and are never modified afterward. Declaring them as mutable storage variables unnecessarily increases gas costs, since they are stored in contract storage rather than being optimized as constants in bytecode.

It is recommended to declare variables that are assigned only once in the constructor as immutable to reduce gas costs:

IUniswapV2Router02 public immutable router;
BBToken public immutable esteem;
uint256 public immutable startTime;
IUniswapV2Pair public immutable pair;
address public immutable token0;
address public immutable token1;
IUniswapV2Pair public immutable pair;
address public immutable token0;
address public immutable token1;

SOLVED: The Better Bank team solved the issue in the specified commit id. Variables were changed to immutable where possible.

Several functions across the contracts accept address parameters without checking against address(0). This can result in unintended behaviors such as minting to the zero address, assigning roles to invalid accounts, or transferring tokens to a non-existent destination. Functions affected include:

• Esteem: addMinter, removeMinter

• Favor: addMinter, removeMinter, mint

• Staking: setTreasuryOperator

• MintRedeemer: mintEsteemWithPLS, redeemFavor

• Zapper: requestFlashLoan, zapToken, sellTo, buyTo

It is recommended to enforce explicit zero-address checks across all user inputs to prevent unintended behaviors.

SOLVED: The Better Bank team solved the issue in the specified commit id. It was added require(account != address(0), "Cannot be address(0)"); to functions.

In the Zapper contract, the zapToken and _zapToken functions eventually call _refundDust, which sends ETH back to the caller using a low-level .call. Without reentrancy protection, this opens the possibility of reentrant calls during the refund step. Other core functions in the contract already use nonReentrant, but it is missing here.

It is recommended to add the nonReentrant modifier to zapToken and _zapToken to prevent reentrant calls during ETH refunds.

SOLVED: The Better Bank team solved the issue in the specified commit id.

In the Zapper contract, the executeOperation function deposits freshly minted Uniswap LP tokens directly into Aave, making Aave the effective holder of the LP supply:

function executeOperation(
        address asset,
        uint256 amount,
        uint256 premium,
        address initiator,
        bytes calldata params
    ) external returns (bool) {
    ........
    ........

    (, , uint256 lpAmount) = router.addLiquidity(
         asset,
         favorToken,
         amount,
         tokenAmount,
         0,
         0,
         address(this),
         block.timestamp + 50
    );

    IERC20(lpToken).forceApprove(address(POOL), lpAmount);

    POOL.supply(lpToken, lpAmount, user, 0);
    POOL.borrow(asset, amount + premium, 2, 0, user);

    IERC20(asset).forceApprove(address(POOL), amount + premium);

    emit FlashLoanExecuted(user, lpToken, lpAmount);

    return true;
}

If borrowing or flash loans were available against this reserve, an attacker could borrow the entire LP supply in a single transaction, redeem it for the underlying tokens, temporarily reset the on-chain LP price, re-add liquidity, and repay the loan. This sequence would allow the attacker to capture the accrued premium that should have represented yield for Aave depositors, effectively draining their interest. While the current protocol does not expose borrowing or flash loan functionality, enabling such features in the future would make this exploit feasible and could result in significant fund loss and token price resets.

It is recommended to lock a minimum amount of LP liquidity so that the entire supply cannot be redeemed and the per-LP price cannot be reset through a flash loan.

ACKNOWLEDGED: The Better Bank team acknowledged this finding.