Heroglyphs A-1

The apxETHVault.sol contract, a variant of a drip vault integrating with Dinero Pirex ETH, uses the native asset deposits with the auto compounding flag true to accrue yield while holding the GOB lot until a winning block, with a previously set guess to be mined by a Heroglyphs validator.

function _afterDeposit(uint256 _amount) internal override {
    pirexEth.deposit{ value: _amount }(address(this), true);
}

Reference: apxETHVault.sol#L37-39

When a winning block is selected, the vault is migrated, or a user claims a winning guess, the GOB contract will withdraw from the vault, which in turn executes the _beforeWithdrawal() logic in the apxETHVault contract:

function _beforeWithdrawal(address _to, uint256 _amount) internal override {
    **uint128 exitedPx = uint128(apxETH.redeem(apxETH.maxRedeem(address(this)), address(this), address(this)));**
    uint256 interestInPx;
    uint256 cachedTotalDeposit = getTotalDeposit();

    **uint256 amountInPx = apxETH.convertToShares(_amount);**
    uint256 exitedInETH = apxETH.convertToAssets(exitedPx);

    //Shares scales down, in full exit, we might find less than the total deposit
    if (exitedInETH > cachedTotalDeposit) {
        interestInPx = apxETH.convertToShares(exitedInETH - cachedTotalDeposit);
    }

    _transfer(address(pxETH), rateReceiver, interestInPx);
    _transfer(address(pxETH), _to, amountInPx);

    if (cachedTotalDeposit - _amount != 0) {
        apxETH.deposit(pxETH.balanceOf(address(this)), address(this));
    } else {
        // Transfer the remaining balance of pxETH to the rateReceiver, left over from shares conversion
        _transfer(address(pxETH), rateReceiver, pxETH.balanceOf(address(this)));
    }
}

Reference: apxETHVault.sol#L41-63

However, the logic does not consider the apxETH → pxETH redeem fees, currently set at 0,03% (but can be changed by the Pirex ETH owner). This causes some undesired outcomes:

As the Vault redeems the maximum balance (maxRedeem()) on each withdrawal:

• If a validator and treasury fee are set, each withdrawal will pay for the 0,03% of the total.
• Each time a user claims a different winning bet, the vault will pay a 0,03% fee on the total assets held by the Vault, including different block and unallocated assets.

Additionally, depending on how much yield has been accrued between deposits and withdrawals:

• If the yield accrued exceeds the equivalent fee, the rewards will cover the fee, and the Heroglyph interest will be lower than expected.
• If the yield is insufficient to cover the fees, the contract will revert as the amountInPx is calculated using _amount and not the balance received after redeeming.

Note that not accounting for the actual received balances could cause rounding issues to deny withdrawals. For example, the current test_onRun_thenFlowWorks() test in GOBPxETHE2E.t.sol currently reverts when updating the vault since the asset-to-share conversion of the total deposits has a 1 wei less discrepancy with the redeemed balance.

Remediations to Consider:

• Only redeem the desired amount and verify the received pxETH.
• Use and transfer apxETH and allow the end receiver to handle redeem or withdrawal fees.

When a Vault migration is initiated through the updateVaultDrip() ownable function, all the current deposits are withdrawn to the treasury address to properly handle withdrawal delays and move the assets to the new vault:

function updateDripVault(address _dripVault) external onlyOwner {
        ...

    uint256 totalDeposit = dripVault.getTotalDeposit();
    dripVault.withdraw(treasury, totalDeposit);

    ...
}

Reference: GuessOurBlockReceiver.sol#L209-221

In AaveVault.sol, the _beforeWithdrawal() function withdraws the maximum withdrawal value from the pool to calculate the interest and then re-supply the difference between the total deposits and the amount.

function _beforeWithdrawal(address _to, uint256 _amount) internal override {
    **uint128 exited = uint128(aaveV3Pool.withdraw(address(weth), type(uint256).max, address(this)));**

    uint256 cachedTotalDeposit = getTotalDeposit();
    uint256 interest = exited - cachedTotalDeposit;

    **aaveV3Pool.supply(address(weth), cachedTotalDeposit - _amount, address(this), 0);**

    weth.withdraw(_amount + interest);

    _transfer(address(0), rateReceiver, interest);
    _transfer(address(0), _to, _amount);
}

Reference: AaveVault.sol#L40-52

Since Aave V3 pools require the amount to be different from zero in the supply() validation logic, the contract will revert with error code “26,” representing an invalid amount. If the initial vault is set to use the AaveVault implementation, any attempt to update the vault will result in a revert.

Remediations to Consider:

Verify if the total amount was used in withdraw() and skip the supply() call if so.

When calling the updateDripVault() ownable function, the total deposits corresponding to the lot will be withdrawn to the treasury address, as mentioned in the comments, due to some vault drips' withdrawal delays.

 function updateDripVault(address _dripVault) external onlyOwner {
    if (permanentlySetDripVault) revert CanNoLongerUpdateDripVault();
    if (_dripVault == address(0)) revert DripVaultCannotBeZero();

    uint256 totalDeposit = dripVault.getTotalDeposit();
    dripVault.withdraw(treasury, totalDeposit);

    dripVault = IDripVault(_dripVault);
    isMigratingDripVault = true;

    emit DripVaultUpdated(_dripVault);
    emit DripVaultMigrationStarted();
}

Reference: GuessOurBlockReceiver.sol#L209-221

This function will set the isMigratingDripVault bool to true. However, no checks in _lzReceive() consider this variable, potentially causing the newly updated vault to revert if a block wins and the contract attempts to withdraw fees or pay the winner.

Remediations to Consider

Consider checking the isMigratingDripVault and returning early if it’s set to true.

As per the LayerZero V2 documentation, the endpoint IDs should not be hard coded, and access control setters should be used to prevent potential integrator changes from breaking the application.

Remediations to Consider:

Consider implementing an onlyOwner setter for the lzEndpointReceiverId.

In GuessOurBlockSender the latestMintedBlock is checked against the passed _blockNumber and returns early if higher.

function onValidatorTriggered(uint32, uint32 _blockNumber, address _validatorWithdrawer, uint128 _heroglyphFee)
    external
    override
    onlyRelay
{
    // return instead a revert for gas optimization on Heroglyph side.
    if (latestMintedBlock > _blockNumber) return;

    ...  
}

Reference: GuessOurBlockSender.sol#L30-47

However, since each Heroglyph validated block can include in the graffiti the same ticker multiple times, it could cause the onValidatorTriggered() function to be called multiple times with the same block, sending a cross-chain message that will return early as the block will be completed on the first call.

Remediations to Consider:

Two potential owner configuration setters could cause undesired revert scenarios, breaking the contract:

Configuring minimumBlockAge to zero:

• This would allow an actor to front-run the LZ message delivery with the won block and place bets that will instantly win. Rendering the bet odds and guess unfair.

Configuring groupSize variable to zero will:

• _lzReceive() to revert in #114:

  uint32 blockNumberTail = blockNumber - (blockNumber % groupSize);
  

• _guess() to revert on the _isValidTailBlockNumber() check in line #98:

  return _tailBlockNumber % groupSize == 0;
  

Remediations to Consider:

Consider having minimum bound values for these configuration setters.

In GuessOurBlock sender, the variable fullWeightCost can be declared immutable since it’s only set in the constructor.

The GuessOurBlockReceiver contract logic has the guess weight unit value hardcoded as 1e18 when calculating the guessWeight and reducedLot:

uint128 guessWeight = uint128(Math.mulDiv(_nativeSent, **1e18**, fullWeightCost));

Reference: GuessOurBlockReceiver.sol#L88

if (blockMetadata.totalGuessWeight < **1e18**) {
   uint128 reducedLot = uint128(Math.mulDiv(winningLot, blockMetadata.totalGuessWeight, **1e18**));

Reference: GuessOurBlockReceiver.sol#L133-134

Consider declaring a constant variable for this to increase readability and make its usage straightforward.

In BaseDripVault.sol contract, the _transfer() internal function is used to transfer native assets or ERC20 tokens depending on the _asset address input:

function _transfer(address _asset, address _to, uint256 _amount) internal {
    if (_amount == 0) return;

    if (_asset == address(0)) {
        (bool success,) = _to.call{ value: _amount }("");
        if (!success) revert FailedToSendETH();
    } else {
        IERC20(_asset).transfer(_to, _amount);
    }
}

Reference: BaseDripVault.sol#L39-48

This function uses the standard IERC20.transfer(). Consider using safeTransfer() instead to handle potential non-standard transfers, such as token transfers that do not revert to failed transfers.

In GuessOurBlockReceiver.sol contract, the receive() function allows the dripVault to send native assets back again to the contract.

receive() external payable {
    if (msg.sender != address(dripVault)) revert InvalidSender();
}

However, the logic will always withdraw to the proper end without going to the GuessOurBlockReceiver. The claim() function will send the assets directly to the user. When executing the lzReceive() logic or claiming, they are sent to other addresses (validator, treasury, or claimants), not back to this contract. Consider removing this logic, as it’s unnecessary with the current vaults and logic.

In GuessOurBlockReceiver.sol contract, the treasury address variable is only set in the constructor. Although not explicitly immutable, this address can not be changed by any method. Consider having an onlyOwner function that allows this address to be updated in case of a potential misconfiguration or migration of treasury or marking this address as immutable.