v3-poc

Description

When creating a claim towards an Alchemist, the Transmuter uses the totalDebt for the Alchemist as a cap. Presumably, this is to ensure, in the multi-Alchemist scenario, that users don't overburden one Alchemist with paying back more debt than that Alchemist has issued, essentially sanctioning other Alchemists.

if (totalLocked + syntheticDepositAmount > IAlchemistV3(alchemist).totalDebt())    revert DepositCapReached();

However, this logic is flawed, since totalDebt tracks how leveraged the Alchemist is, not how many synthetic tokens are outstanding from it (and thus need to be accounted for).

Specifically, repay() will reduce the totalDebt and send yield tokens to the Transmuter, hence making sure that the synthetic tokens it has issued are fully redeemable. But the totalDebt check will prevent redemptions in the Transmuter from being created if the syntehtic tokens have been "paid off", due to the failure to account for the yield tokens sent to the Transmuter.

Proof of Concept

In AlchemistV3.t.sol:

function testLoanRepayAndRedemption() external {    address user = address(0xbeef);    uint256 ownedAmount = 100e18;    uint256 depositAmount = ownedAmount / 2;
    // Deposit yield tokens to get a position    vm.startPrank(user);    SafeERC20.safeApprove(address(fakeYieldToken), address(alchemist), ownedAmount);    alchemist.deposit(depositAmount, user, 0);    uint256 nftId = AlchemistNFTHelper.getFirstTokenId(address(0xbeef), address(alchemistNFT));
    // Mint synthetic tokens    uint256 mintAmount = alchemist.getMaxBorrowable(nftId) / 2;    alchemist.mint(nftId, mintAmount, user);
    // Repay the loan with yield tokens    uint256 repayAmount = alchemist.convertDebtTokensToYield(mintAmount);    alchemist.repay(repayAmount, nftId);
    // Create redemption in Transmuter    SafeERC20.safeApprove(address(alToken), address(transmuterLogic), mintAmount);
    transmuterLogic.createRedemption(mintAmount); // [FAIL. Reason: DepositCapReached()] 
    vm.stopPrank();}

This test imagines a scenario with a single user. They deposit yield tokens, takes out a loan, then repays their loan with their synthetic tokens. In this scenario, they then try to redeem them through the Transmuter. In reality, they may have sold the tokens in the open market, and some other user is looking to redeem them to e.g. close an arbitrage. However, the redemption cannot be created because the Alchemist has no outstanding debt and totalDebt == 0.

Recommendation

If the protocol is designed to have only one Alchemist-Transmuter pair per syntetic token, then the check can be removed: all synthetic tokens should be redeemable through the one Transmuter.

If the intention is to have multiple Transmuters, even just during migration from V2, then it is better keep track of the total outsanding tokens issued by the Alchemist in question, separately from totalDebt, and use that for the above check.

Description

The createRedemption function in Transmuter does not validate whether the yieldToken provided by the user is actually associated with the specified alchemist. Additionally, during redemption, there is no such validation either.

ITransumter.createRedemption is specified to take underlying as an input parameter. The assumption that all underlying assets are worth the same and should convert at a 1:1 exchange rate might be correct. However, the actual implementation allows users to provide a yieldToken instead of underlying, and different yieldTokens do not have the same value.

An attacker can exploit this by:

1. Creating a redemption request using an alchemist associated with a lower-value yieldToken but inputting a more valuable yieldToken in the function arguments.
2. During redemption, calculations are performed based on the cheaper yieldToken, but the attacker ultimately receives the more valuable token, leading to a loss for the protocol.

Additionally, the provided yieldToken can be an attacker-controlled ERC20 token, potentially allowing re-entrancy or manipulation via balanceOf calls.

Recommendation

Ensure that the yieldToken provided by the user is explicitly associated with the specified alchemist.

Description

The following line is used to calculate the amount of debt left to be transmuted in a position:

uint256 blocksLeft = position.maturationBlock > block.number ? position.maturationBlock - block.number: 0;uint256 amountNottransmuted = blocksLeft > 0 ? position.amount * blocksLeft / timeToTransmute : 0;uint256 amountTransmuted = position.amount - amountNottransmuted;

The global variable timeToTransmute determines the duration that new redemptions must wait before completely transmuting. According to the team, the Transmuter is designed so that the total time required for maturity is established when a redemption is created.

By using the variable timeToTransmute here, the maturation time for an existing position is affected when the global variable is updated, which is not intended.

Furthermore, as the correct maturation times are used for updating and querying the staking graph, the earmarking accounting would become incorrect, and either too little or too much would be earmarked as timeToTransmute changed.

Proof of Concept

timeToTransmute is set to 200,000 blocks, roughly 28 days.

User A creates a redemption for 10,000 alUSD at block T. The maturation block is set to T + 200,000.

A week and 50,000 blocks later, due to increasing yield and an unstable alUSD peg, timeToTransmute is halved to 100,000 blocks.

User A decides to exit because they need liqudity. Their position has matured for a week, so their remaining position should be 7,500 alUSD, and they should receive the rest (valued at 2,500 USD) in yield tokens.

Due to the above calculation, however, their remaining position will be calculated to

uint256 blocksLeft = T + 200_000 > T + 50_000 ? T + 200_000 - (T + 50_000) : 0;    // 150_000uint256 amountNottransmuted = 150_000 > 0 ? 10_000 * 150_000 / 100_000 : 0;  // 15_000uint256 amountTransmuted = 10_000 - 15_000;  // Underflow!

Because timeToTransmute halved, User A's remaining debt immediately doubled to 15,000, which is higher than their original deposit. As a result of transmutation times in general decreasing, theirs, and all existing positions, had their maturation time increased. Used A would now have to wait another week until they even start transmuting.

Another week and 50,000 blocks go by. It's been a total of 100,000 blocks since Users A and B created their positions. At this time, timeToTransmute is increased to 400,000 blocks.

At this point, User A decides to try to exit again. Their position should be half matured, and they should have approximately 5,000 debt tokens left to transmute and receive yield tokens worth 5,000 USD, minus fees.

However, they get the following calculation.

uint256 blocksLeft = T + 200_000 > T + 100_000 ? T + 200_000 - (T + 100_000) : 0;    // 100_000uint256 amountNottransmuted = 100_000 > 0 ? 10_000 * 100_000 / 400_000 : 0;  // 2_500uint256 amountTransmuted = 7_500 - 2_500;  //

User A thus is considered 3/4ths matured. As a result of maturation times generally increasing, theirs went down significantly.

Recommendation

Further down in the code, there is the following line:

uint256 transmutationTime = position.maturationBlock - position.startBlock;

Move that line up to before the problematic line and replace timeToTransmute in the denominator with transmutationTime.

Description

After discussions with the team, we concluded that the correct design is to make Transmuters and Alchemists one-to-one. In Alchemix v2, having multiple underlying tokens was handled by having different Alchemists for each underlying yield token. In v3, this diversity of collateral is handled at the vault level, so that the yield token the Alchemist deals with can consist of any basket of underlying tokens the controlling DAO authorizes, through the managed Euler Eearn vault. Hence the need for multiple Alchemists per synthetic token (and hence per Transmuter) goes away.

The current design would not be able to correctly handle several Alchemists, since each Transmuter has only one staking graph, which the Alchemist(s) query to determine how much to earmark. If there were several Alchemists they would earmark extra funds for each other.

There is also a related issue with several alchemists, reported with the title "Incorrect index reassignment in removeAlchemist prevents future removals"

Recommendation

Redesign the Transmuter to use only one Alchemist and one yield token. Remove the alchemist and yieldToken parameters from createRedemption(). Also remove these fields from the StakingPosition struct.

Description

The _baseURI() function in AlchemistPosition is not overridden, resulting in empty NFT images and metadata in wallets and portfolio sites. These platforms rely on tokenURI to fetch NFT metadata, so without a valid URI, NFTs will appear blank.

Recommendation

Override _baseURI() and provide at least a placeholder URI like nftMetadata.alchemix.fi/ that returns a logo. Additionally, consider including useful metadata such as the NFT’s value in underlying terms. For a fully on-chain solution, you can take inspiration from UniswapV3 and SablierV2.

Description

The Alchemist allows swapping out Transmuters through the setTransmuter() function. However, sunsetting a Transmuter without simultaneously sunsetting (all) its Alchemist(s) would be very difficult to do in a way that does not harm creditors with debt tokens in the Transmuter.

The Transmuter relies on its privileged access to it's Alchemist(s) in order to redeem yield tokens to pay back creditors. In the event that it loses this privilege, redemptions will fail for any position that the Transmuter cannot cover. The value in the Transmuter would likely be drained by new or old creditors entering and exiting positions until all yield tokens have been siphoned off.

Recommendation

Only sunset a Transmuter, which is in some way buggy and absolutely needs swapping out. Make it possible to lower the deposit cap arbitrarily, even to 0, to block new deposits (currently, it cannot be lowered below the current totalLocked in the Transmuter). Make the Transmuter creditors whole with emergency funds, if available.

Description

The current calculation results in zero fees for liquidating accounts with bad debt. This removes any incentive for liquidators to act, meaning that positions where debt exceeds collateral will not be cleared. Over time, these bad debt positions will continue to deteriorate, increasing protocol risk.

Recommendation

• Remove the conditional logic that prevents fees from being paid when debt exceeds collateral. Always pay feeInDebt.
• Track when fees exceed remainingCollateral and replenish the transmuter from a DAO security fund when necessary.

Description

The current event indexing seems inconsistent from a usability perspective and makes it hard to monitor a position via events.

Recommendation

On all the events that affect single positions, add indexation on the token ID affected.

Remove the indexation on BatchLiquidated. The topic would be based on the keccak hash of the full array of liquidated accounts, so it would only be possible to match the full array of liquidated accounts and not search by any one account.

Think about utilizing a Liquidated event, even for batch liquidations, to enable tracking of all events impacting a position.

Remove the Liquidate event, which is unused.

Description

The Transmuter admin is set once and cannot be changed, which looks like an oversight. The contract deployer has to be the admin forever which limits governance flexibility and could lead to issues if admin privileges need to be transferred.

Recommendation

Implement setPendingAdmin and acceptAdmin methods, similar to AlchemistV3, to allow controlled admin updates.

Description

The Transmuter contract keeps track of allowed alchemists using an array and a mapping (alchemist address => index). When an alchemist is removed, the last element in the array is moved to its position before calling pop. However, the index of the last alchemist is not updated in the mapping.

As a result, this last alchemist's recorded index becomes greater than the array size, leading to an out-of-bounds error when attempting to remove it later.

Proof of Concept

function testRemoveAlchemistBug() public {        address alchemistA = address(0xA);        address alchemistB = address(0xB);
        transmuter.addAlchemist(alchemistA);        transmuter.addAlchemist(alchemistB);        (uint256 alchemistBIndexBefore,) = transmuter.alchemistEntries(alchemistB);
        (uint256 alchemistAIndex,) = transmuter.alchemistEntries(alchemistA);
        transmuter.removeAlchemist(alchemistA);
        assertEq(transmuter.alchemists(alchemistAIndex), alchemistB); //this is expected that the last alchemist is moved to the removed alchemist's index        (uint256 alchemistBIndex,) = transmuter.alchemistEntries(alchemistB);
        assertEq(alchemistBIndex, alchemistBIndexBefore); //alchemistB's index should not be the same as before. It should be replaced with removed alchemist's index
        //This is the impact of the bug. Now, alchemistB cannot be removed because it panics with array out-of-bounds access        vm.expectRevert();        transmuter.removeAlchemist(alchemistB);    }

Recommendation

When swapping the last element into the removed alchemist’s position, update its index in the mapping.

Description

The wrap and unwrap functions are unused. Furthermore, the code as it is is unsafe because wrap() double-sends tokens with both deposit() and safeTransfer(). unwrap() behaves similarly.

Furthermore the USDC adapter uses a hard-coded 6 decimals to compute price, and thus the code should not be copied for another type of adapter without first modifying the decimals.

Recommendation

Remove the unwrap() and wrap() functions.

Description

The deposit cap cannot be lowered below what the current amount of totalLocked is.

function setDepositCap(uint256 cap) external onlyAdmin {    _checkArgument(cap >= totalLocked);    // ...}

This seems overly restrictive.

The only other use of the deposit cap is at line 177, which would not be buggy if you removed this restriction. If you want to lower the deposit cap (e.g. because you're switching out transmuters and would prefer to not allow more deposits) you should be able to.

Recommendation

Remove the line in question.

Description

The following line updates the staking graph with withdrawals at the end of claimRedemption():

if (amountNottransmuted > 0) _updateStakingGraph(-position.amount.toInt256() * BLOCK_SCALING_FACTOR / transmutationTime.toInt256(), blocksLeft);

However, there is no limit to how small a redemption position can be. So there is no guarantee that amountNottransmuted will be 0 only when the position is fully matured.

Proof of Concept

User A creates many small redemptions of 100 tokens (with 18 decimals, for e.g. alUSD this is 10^-16 USD worth of tokens).

If timeToTransmute is 200,000 blocks (~28 days), then the calculation proceeds as follows after waiting for 198,001 blocks:

uint256 amountNottransmuted = blocksLeft > 0 ? position.amount * blocksLeft / timeToTransmute : 0; // 100 * 1_999 / 200_000 = 0

Now, if there are many redemptions maturing in the same block, for example, if User A created 100 of these redemptions, then the staking graph will still contain a diff of 100 tokens at the remaining blocks that will not be updated.

Recommendation

Use blocksLeft instead of amountNottransmuted:

if (blocksLeft > 0) _updateStakingGraph(-position.amount.toInt256() * BLOCK_SCALING_FACTOR / transmutationTime.toInt256(), blocksLeft);

Description

Currently, MintAllowances are attached to token IDs.

• Owner A of token ID x grants an allowance of a certain amount to address P.
• Owner A transfers the NFT (ID x) to owner B.

Here, Address P still retains permission to mint debt tokens using ID x, even after the transfer.

Transferring NFTs does not reset the allowances. This could be problematic if someone assumes the usual behavior of NFTs, where allowances are typically reset upon ownership transfer.

Recommendation

Reset MintAllowances on NFT transfer.

Description

If depositCap is set to a very high value, users can manipulate the globalCollateralizationRatio by strategically depositing and withdrawing yieldTokens:

• To lower globalCollateralizationRatio

  • Deposit a large amount of yieldTokens.
  • Perform a liquidation.
  • Withdraw the yieldTokens.
  • (This benefits users liquidating their own positions.)

• To increase globalCollateralizationRatio

  • Deposit a large amount of yieldTokens.
  • Mint the maximum allowed alAssets.
  • Perform liquidation.
  • Repay the alAssets and withdraw the yieldTokens.
  • (This benefits liquidators, maximizing fees.)

Recommendation

• Impose reasonable depositCap limits to prevent excessive deposits

Description

The EulerAdapter.price function hardcodes 1e6 when calling the underlying vault. While this works for USDC (6 decimals), it fails for tokens with different decimal places. If deployed for other tokens, the price calculation will be incorrect.

Recommendation

Instead of using a fixed 1e6, dynamically determine the appropriate value based on the token’s decimals:

function price() external view returns (uint256) {    uint8 decimals = TokenUtils.expectDecimals(token);    return IERC4626(token).convertToAssets(10**decimals);}

Ensure this update is made before deploying this adapter for non-6-decimal tokens to prevent incorrect price calculations.

Description

Currently, the URI does not include chainId or transmuter address, which can cause issues when deploying multiple contracts across different chains. The server that returns ERC1155 metadata won't be able to determine which chain’s or which transmuter's metadata to return for the same token ID, leading to incorrect or missing data.

Recommendation

Append chainId and transmuter address to the URI to ensure each metadata request is correctly mapped to the appropriate chain and transmuter.

Description

Positions are attached to both an address and an ID, which restricts the transferability of ERC1155 tokens. Additionally, tracking the amount in StakingPosition.amount further limits the fungibility of ERC1155 tokens with the same ID. A user is forced to transfer their entire ERC1155 balance for a specific ID to another wallet; otherwise, they cannot call claimRedemption.

To resolve this, avoid tracking the amount in StakingPosition.amount and instead allow the user to specify the amount as an input when calling claimRedemption.

Proof of Concept

function testClaimRedemptionERC115TransferNotAllowedBug() public {        deal(address(collateralToken), address(transmuter), uint256(type(int256).max) / 1e20);
        vm.prank(address(0xbeef));        transmuter.createRedemption(address(alchemist), address(collateralToken), 100e18);
        vm.roll(block.number + 5_256_000);
        //The original creator send the ERC1155 tokens to a new wallet        vm.prank(address(0xbeef));        transmuter.safeTransferFrom(address(0xbeef), address(0xABC), 1, 100e18, "");
        //this new wallet cannot claim redemption because positions are attached to an address + id. It should only be attached only ID        vm.startPrank(address(0xABC));        vm.expectRevert(PositionNotFound.selector);        transmuter.claimRedemption(1);        vm.stopPrank();    }

Recommendation

There is no need to track ownership of a position—let the ERC1155 token contract handle it. Additionally, do not track the amount in StakingPosition.amount. Instead, allow the user to specify the amount when calling claimRedemption. This ensures that users can transfer ERC1155 tokens freely without losing value.

Description

In the AlchemistV3.Deposit event, recipientId provided by user does not represent the actual recipient in cases where recipientId = 0. When the user provides recipientId = 0, a new alchemistPositionNFT is minted, and the deposit is associated with the newly minted ID.

Recommendation

Modify the event to emit tokenId instead, ensuring that the correct recipient ID is reflected in the event.

Description

Solidity 0.8.27 allows the use of custom errors directly in require statements without needing the Yul (IR) pipeline. This reduces bytecode size and enables more informative errors with parameters.

Recommendation

Refactor require statements to use custom errors for improved efficiency and clarity.

Description

Alchemix v3 should only use yield tokens which revert correctly on failure to transfer, and always transfer the full amount. They must not, for example, issue fees or be able to block transfers to and from certain addresses (unless controlled by the DAO controlling Alchemix), since the transfers of yield tokens do not perform a post-check that the final balance is correct but instead only assumes that a call to TokenUtils.safeTransferFrom() will execute correctly or revert.

The synthetic token is assumed to be the Alchemic tokens already deployed and in use in v2, or newly deployed such tokens with the same contract code.

Recommendation

Only use Euler Earn tokens under the DAO's control, as currently planned, or revisit all token transfer locations in the code before adding support for additional yield tokens.

Description

Incorrect capitalization in the import

import {AlEth} from "../external/Aleth.sol";

Recommendation

Capitalize the final "E":

import {AlEth} from "../external/AlEth.sol";

Description

There are many instances of the misspelling "gaurdian".

Recommendation

Change everywhere to "guardian".

Description

There are several places where the literal 1e18 is used instead of the named constant FIXED_POINT_SCALAR. This reduces readability and makes it unclear what the multiplication or division is intended to do.

Recommendation

Change all usages of 1e18 to FIXED_POINT_SCALAR.

Description

The way to create a new position in the Alchemist is to make a deposit with tokenId 0.

if (tokenId == 0) {    tokenId = IAlchemistV3Position(alchemistPositionNFT).mint(recipient);    emit AlchemistV3PositionNFTMinted(recipient, tokenId);}

However this is not prominently documented in the NatSpec, and is only specified in the code.

Recommendation

Add documentation on this behavior.

Description

Once multiple deposits have been made, smoothly replacing the AlchemistPositionNFT contract from Alchemist becomes impractical. Existing positions are tied to the current NFT contract, and switching it could lead to inconsistencies or loss of position tracking.

Recommendation

Remove the setAlchemistPositionNFT method to prevent unexpected changes that could break existing positions.

Description

The contract does not emit an event when transmutationTime is set in setTransmutationTime

Recommendation

Add the missing event.

Description

The codebase can be cleaned up and made more readable by addressing several small issues:

1. Resolve remaining TODO comments: IAlchemistV3.sol#L28
2. Use named imports for clarity: AlchemistV3Position.sol#L4
3. Use _checkState instead of _checkArgument for minimumCollateralization, as it is a state variable. AlchemistV3.sol#L890
4. Use named constants like FIXED_POINT_SCALAR instead of hardcoded values like 1e18: AlchemistV3.sol#L890
5. Fix spelling mistake: expectedColltaeralForCurrentDebt → expectedCollateralForCurrentDebt AlchemistV3.sol#L901
6. Simplify boolean checks: Replace _checkState(depositsPaused == false); with _checkState(!depositsPaused); for minor gas savings. AlchemistV3.sol#L325
7. Correct the comment: _isUnderCollateralized returns true if the account is undercollateralized and false otherwise. It doesn't revert with an {Undercollateralized} error. AlchemistV3.sol#L869
8. Remove unused imports:
   • AlchemistV3Position.sol#L5
   • StakingGraph.sol#L6
9. Remove unused state variable: AlchemistV3.sol#L38
10. Resolve compiler warnings important contracts

Recommendation

Update the code as recommended.

Description

The following issue was identified by the Alchemix team shortly before the audit started. The design to fix it is being worked out, and the audit was conducted with this in mind: the earmarking and syncing logic may change in response to how this bug should be addressed.

The weight system used to account for earmarked debt does not properly handle scenarios where the total supply of debt tokens decreases, leading to inaccurate bookkeeping of user debt positions.

In the Alchemist system, users' debt decreases when the Transmuter requests collateral from the Alchemist. When a redemption is claimed from the Transmuter, tokens held in escrow are sent to the position owner, and an equal amount of debt is removed from the system. To correctly account for what portion of earmarked debt belongs to which user, the system uses a global weight that users can sync their accounts against.

These weights represent the ratio of debt that has been earmarked, calculated as:

$\text{New Weight} = \frac{\text{Newly Earmarked Amount}}{\text{Total Supply of Debt Token}} + \text{Old Weight}$

The issue occurs because the denominator ($\text{Total Supply of Debt Token}$) decreases when redemptions or burns occur, causing the weight calculation to become inaccurate. This is demonstrated in the testPositionToFullMaturityTwoTransmuterPositions() test where:

1. A user mints 100,000 tokens from the Alchemist
2. A user stakes 50,000 tokens in the Transmuter
3. After 1/4 of the transmutation period, another user stakes 50,000 tokens
4. After a full transmutation period, 87,500 tokens are earmarked (50,000 for the first position, 37,500 for the second), giving a weight of 0.875
5. When the first redemption occurs, the total debt decreases to 50,000
6. When the final 12,500 tokens are earmarked, the weight is incorrectly incremented by $\frac{12,500}{50,000}$, resulting in a weight of 1.125

This value suggests the position has more debt tokens earmarked than were minted, which is impossible. The correct increment should be $\frac{12,500}{100,000}$, yielding a weight of 1, representing that all tokens are earmarked.

The fundamental issue is that the denominator for the ratio calculation changes over time due to redemptions, making it impossible to maintain an accurate weight that works for all users across all scenarios.

Description

The check for if alchemistPositionNFT != address(0) is not necessary in the deposit(), withdraw(), mint() and burn() functions because they call the NFT contract to check ownership and validity, and those calls will revert if calling to an address with no contract. Since it's a very abnormal state, one that can only happen if the NFT contract has never been set at all, it is wasteful to perform this check every time.

Proof of Concept

In the AlchemistV3.t.sol tests, change the setUp() to not set an NFT contract. Remove the check in question in deposit(). Run the following test:

function testDeposit() public {    uint256 amount = 10;    vm.startPrank(address(0xbeef));    SafeERC20.safeApprove(address(fakeYieldToken), address(alchemist), amount + 100e18);    alchemist.deposit(amount, address(0xbeef), 0);    vm.stopPrank();}

The test reverts. If the NFT contract is left to be set, the test passes.

Recommendation

Remove the unnecessary check. If you insist on using it for clarity, change _checkArgument() to _checkState().

Description

Unnecessary if-clause:

if (collateralization < minimumCollateralization) {    return true;}
return false;

Recommendation

return collateralization < minimumCollateralization;

Description

There is no need to check that the sender is not the 0-address, since the 0-address cannot be msg.sender on Ethereum or any major L2.

There is, however, no check that recipient is not address(0).

Recommendation

Remove the line

_checkArgument(msg.sender != address(0));

If desired the check can be changed to

_checkArgument(recipient != address(0));

Description

Consider the following line in the Transmuter:

uint256 amountNottransmuted = blocksLeft > 0 ? position.amount * blocksLeft / timeToTransmute : 0;

If blocksLeft is indeed 0, the calculation position.amount * blocksLeft / timeToTransmute gives the result 0. The branching logic is unnecessary.

Recommendation

Change to:

uint256 amountNottransmuted = position.amount * blocksLeft / timeToTransmute;

Description

Every time a redemption is created in the Transmuter, the following checks are performed.

if ( syntheticDepositAmount > type(int256).max.toUint256())    revert DepositTooLarge();
if (totalLocked + syntheticDepositAmount > depositCap)    revert DepositCapReached();

The second check should trigger far more often than the first.

Recommendation

A similar result can be achieved by making a check that depositCap can never be larger than type(int256).max.toUint256(), and thus totalLocked can't either. This would prevent similar issues while only resulting in an overflow revert if someone submits a very large number instead of an error.

Description

There is a check for valid accounts performed when depositing:

if (tokenId == 0) {    tokenId = IAlchemistV3Position(alchemistPositionNFT).mint(recipient);    emit AlchemistV3PositionNFTMinted(recipient, tokenId);}_checkForValidAccountId(tokenId);

However, if tokenId == 0 the token will have just been created. This NFT contract is trusted code.

_checkForValidAccount() makes an external call, and is thus fairly costly.

Recommendation

Put the validity check in an else block.

if (tokenId == 0) {    tokenId = IAlchemistV3Position(alchemistPositionNFT).mint(recipient);    emit AlchemistV3PositionNFTMinted(recipient, tokenId);} else {    _checkForValidAccountId(tokenId);}

Description

The ERC721 standard specifies that an NFT with owner == address(0) is invalid, and ownerOf should revert in such cases. Since AlchemistV3Position correctly inherits OpenZeppelin's ERC721 implementation, a successful ownerOf call guarantees that the owner is not the zero address.

Thus, the check (owner != address(0)) is redundant and can be removed.

Recommendation

Remove the unnecessary check as recommended.