Zellic - Audit Reports

Exploitation scenarios considered

Here, we aim to provide a summary of the possible exploitation scenarios we considered for the provided codebase.

ERC-4626 inflation attack

Since the given contract functions as a vault, there is potential attack surface for ERC-4626 vulnerabilities such as the inflation attack. However, the contract implements its vault functionality by inheriting from the OpenZeppelin ERC4626 contract. The project is using V4.9 of the OpenZeppelin contract, which offers out-of-the-box protection from ERC-4626 inflation attacks by using virtual decimal offsets. This makes it safe against the standard ERC-4626 attack vectors.

Share accounting errors

The contract accomodates functionality for minting and burning shares according to user withdrawals and deposits. We considered the following scenarios for the accounting mechanism:

Exploiting share-price manipulation with flash loans
Encountering situations where some shares are non-redeemable.
Minting or double minting unaccounted shares
Burning incorrect shares after withdrawals
Stealing other users' shares

Token considerations

Since the underlying asset might be ERC-777 compliant, we checked for any possible impact reentrancy vulnerabilities might have. In conclusion, the following solution taken from the contract MultiUserLPStakingStrategy.sol behaves as documented and maintains valid state in the case of reentrancy via transfer hooks in ERC-777:

function _deposit(address caller, address receiver, uint256 assets, uint256 shares) internal override {
    // If _asset is ERC777, `transferFrom` can trigger a reentrancy BEFORE the transfer happens through the
    // `tokensToSend` hook. On the other hand, the `tokenReceived` hook, that is triggered after the transfer,
    // calls the vault, which is assumed not malicious.
    //
    // Conclusion: we need to do the transfer before we mint so that any reentrancy would happen before the
    // assets are transferred and before the shares are minted, which is a valid state.
    // slither-disable-next-line reentrancy-no-eth
    IERC20(asset()).safeTransferFrom(caller, address(this), assets);

    (uint256[] memory assetAmounts, address[] memory assetAddresses) = (new uint256[](1), new address[](1));
    (assetAmounts[0], assetAddresses[0]) = (assets, asset());

    IERC20(asset()).resetAndSafeIncreaseAllowance(address(this), address(VAULT), assets);
    VAULT.deposit(assetAmounts, assetAddresses);

    if ((shares = previewDeposit(assets)) == 0) {
        revert ZeroShares();
    }

    // Calculate shares AFTER adding liquidity and BEFORE minting
    // `previewDeposit` calculates using the amount staked before additional liquidity is added
    VAULT.stake(assets);

    _mint(receiver, shares);

    emit Deposit(caller, receiver, assets, shares);
}

function _withdraw(
    address caller,
    address receiver,
    address _owner,
    uint256 assets,
    uint256 shares
) internal override {
    if (caller != _owner) {
        _spendAllowance(_owner, caller, shares);
    }

    // If _asset is ERC777, `transfer` can trigger a reentrancy AFTER the transfer happens through the
    // `tokensReceived` hook. On the other hand, the `tokensToSend` hook, that is triggered before the transfer,
    // calls the vault, which is assumed not malicious.
    //
    // Conclusion: we need to do the transfer after the burn so that any reentrancy would happen after the
    // shares are burned and after the assets are transferred, which is a valid state.
    _burn(_owner, shares);

    VAULT.unstake(assets);
    VAULT.withdraw(assets, asset());

    IERC20(asset()).safeTransfer(receiver, assets);

    emit Withdraw(caller, receiver, _owner, assets, shares);
}

One crucial observation is that similar precautions are not taken for the other tokens that the contract interacts with (such as those in _underlyingAssets); however, we have not flagged this as a security issue as no active exploitation scenario was identified during the audit.