Token Locker

A locker owner can drain the contract of its locked tokens through reentrancy because the legitimacy of the Aerodrome LP is not checked correctly.

When a token is deposited via lockTokens, the implementation contract is called to check that the token is a legitimate Aerodrome LP token created by the Aerodrome factory. The code to check the token is as follows:

    function supportsToken(address tokenContract) external view override returns (bool) {
        return IAerodromePool(tokenContract).factory() == AERODROME_FACTORY;
    }

This does not effectively check that the token is an LP created from the Aerodrome factory as the token contract can simply create a factory function that returns the AERODROME_FACTORY address.

The locker contract can be drained in the following steps:

1. Owner creates a legitimate lock with legitimate token, X. The lock will last 24 months.
2. Owner creates an illegitimate lock with malicious token, Y. The lock period doesn’t matter. Y.token0() will equal token X (the LP token). Y.token1() does not matter.
3. Owner calls collectFees() on lock for token Y. This calls token Y’s claimFees() function which will call UllrLocker.lockTokens() with the legitimate token X and a short 1 second lock for the same amount as the first lock. The increased Locker balance of X will be distributed to the owner as fees, even though it’s actually the amount of the new lock.
4. Owner waits one second and withdraws from the newest lock. The owner now has all of their token X.

Note that the owner would lose some value to protocol fees if the fee value is set to a positive value.

After further review, this attack could still take place even with legitimate Aerodrome LP pool tokens. An attacker could create an Aerodrome LP token using another Aerodrome LP token as token0 and a token with transfer hooks as token1. Therefore, the LP token legitimacy check will not mitigate this vector.

In the supportsToken function, make a call to the Aerodrome Factory's isPool function to verify the legitimacy of the LP token. Furthermore, add nonReentrant modifiers to state-changing functions such as lockTokens and collectFees.

Since Aerodrome has multiple pool factories, a call to the FactoryRegistry.sol contract must be made to ensure that the returned Factory value is legitimate via isPoolFactoryApproved(). Upon verification, then the specific pool must be verified via the isPool on the factory contract.

The arguments which determine the address of the locker contract are independent of the owner of the resulting locker. As a result, in the event of a chain re-org it may be possible for an attacker to steal funds which the user would have locked in their own locker.

Consider the following order of events:

• User A Creates a locker at address A in Block 100
• User A Creates a transaction to approve the locker and sends it to the mempool
• User B observes that a re-org is occurring and is able to create the same locker at address A (using the same salt), but with User B as the owner
• The re-org orders User B’s transaction before Block 100 and User B owns the locker at address A
• User A’s approval transaction still goes through and User B is able to steal the approved amount with the lockTokens function, passing User A as the from address

Furthermore, the creation of a locker could be DoS’d given that any actor can simply provide the same salt to create the locker address before a user’s transaction is recorded.

Consider hashing the provided salt with the address of the owner so that each locker is unique to the owner address and therefore cannot be created at the same address with a different owner.

In the getClaimableFees function when userIndex == 0 && globalIndex > 0 is true then the users index is assigned to the global index, thereby ignoring any fees which have accumulated between the userIndex and the global index.

This is done to account for any LP token holders who have not yet been stamped with the latest index, however this should not be possible since the latest index is written in _updateFor on every token transfer in the _beforeTokenTransfer function.

As a result of this logic in the getClaimableFees function however, LPs which are the first provider when the index is zero will receive an inaccurate reporting of any fees generated before they update their position. For an initial LP locked the getClaimableFees view function will report zero and will not update until the fees are claimed or a new lock with the same token is made to update the index of the user to be nonzero.

Consider removing the handling for the userIndex0 == 0 && globalIndex0 > 0 case since if an account holds an LP token balance it is updated to the latest index upon transfer.

The UllrAerodromeImplementation.sol contract includes a function getClaimableFees that returns the fees that can be claimed from an Aerodrome pool contract. It recreates the reward logic by taking the difference of the locker contract's supply indexes compared to the pool's global supply indexes.

The issue is that it does not account for the locker contract's claimable0 and claimable1 mapping value. In the case where the supply checkpoints are updated for the locker contract through operations such as creating and new lock or topping up an existing lock, the getClaimableFees function will mistakenly return lower than expected values.

Include the locker contract's claimable0 and claimable1 values in the getClaimableFees function.

In the rare case where 16,777,215 locks have been created, the nonce will overflow the 3 bytes allocated for the salt portion of the lock id. In this case it may be possible for a malicious actor to overwrite a user’s lock if they’re able to match the token, lockUntil, and withdrawalDelay of the lock and cause a lock id collision.

On chains like Ethereum this would be clearly cost prohibitive, however in some environments where gas is extremely cheap a malicious actor may be able to force such a collision to occur by creating many dummy locks with dust value.

Validate if the resulting lockId in the lockTokens function already belongs to a lock entry with a nonzero amount entry, and if so, revert.

In the upgradeImplementation function the withdrawalAllowedAt is validated to be zero to ensure that locks which have already had their withdrawals triggered cannot be upgraded.

However for locks which are technically claimable and have a withdrawal delay of zero, they can still be upgraded while being immediately claimable. This is because the _requireWithdrawable function skips the withdrawalAllowedAt validation if the lock withdrawal delay is 0.

Consider using the _isWithdrawable function to validate whether a lock is immediately claimable in the upgradeImplementation function.

In the UllrLocker contract the topUpLock function does not validate if the implementation contract of the lock is still enabled in the factory before allowing the user to add more to their lock.

Consider if this is the expected behavior, or if the topUpLock function should use the isImplementationRegistered function to validate that the implementation is indeed active.

The default protocol fee is assigned as 25 basis points and the MAX_PROTOCOL_FEE* *is assigned as 1000 basis points, therefore there is a wide range of fees which may be charged depending on when the UllrLocker is created.

However there is no slippage validation to ensure that the _protocolFee value used to create the locker clone with immutable args is the value that the creator expected it to be when they originally transmitted the transaction.

As a result, the owner of the UllrFactory contract may frontrun transactions that invoke the createLocker function and unexpectedly raise the protocol fee amount to the maximum value with the setProtocolFee function. Unsuspecting users may not realize this and end up paying the maximum 1000 basis points fee for the entire duration of their lock when they instead intended to accept a much lower fee rate.

Consider adding a fee slippage parameter which allows the user to specify what threshold of fee they are willing to accept.

The owner of the UllrFactory contract may assign a fee receiver which happens to be a blacklisted address for one of the fee tokens and therefore DoS’s all withdrawals and fee claims.

Be aware of this, if this edge case is desired to be mitigated to remove all necessary trust then consider allowing the protocolFeeAmount to be made claimable to the feeReceiver if the token transfer fails instead of reverting with safeTransfer.

The emergency withdrawal flow removes the LP tokens from the locker contract without claiming the fees for the LP, which is intended. However now the fees for the lock that was withdrawn cannot be claimed since the lockInfo struct has been deleted for the lock.

There are ways of rescuing the fees by creating another lock for the contract for the same LP token, however it may be preferrable to add a separate collectFees function which allows the owner to claim fees outside of a specific lock.

Consider adding an onlyOwner collectFees function which allows the owner to collect fees for a given token, irrespective of the lock.

The only requirement for a lock's total duration (lock duration + withdrawal delay) is that at least one of the values must be non-zero. This allows total lock durations as little as 1 second.

Consider adding a minimum duration check, for example, 1 month.

The locker owner can upgrade the implementation contract used for a token deposit. However, when upgrading the implementation, it does not perform the same sanity checks as upon initial locking. This can result in reverts due to incompatibility when attempting to claim fees as part of the withdrawal process.

Implement the supportsToken check in upgradeImplementation.

if (!IUllrImplementation(implementation).supportsToken(token)) {
            revert UllrLocker__UnsupportedToken();
        }

The UllrLocker.sol contract inherits from Ownable.sol, meaning it contains functions for transferring and renouncing ownership. In the case that ownership is accidentally or maliciously renounced, the LP tokens will remain locked in the contract forever.

Consider overriding renounceOwnership() and revert.

In the UllrLocker contract safeTransferFrom is used with an arbitrary from address indicating that users must approve the UllrLocker contract prior to performing their lock or topUp action through an integrating protocol.

It is crucial that this approval is made in the same transaction in which the lock or topUp action is carried out by the integrating protocol, otherwise depending on the integration, the approval amount could be used to fund another lock made by a malicious user.

Be sure to document this risk for integrating protocols who would choose to expose the Ullr locking mechanism to their users.

In the withdrawal flow the _collectAndProcessFees function is used to claim fees to the owner address, however since the fee claim is not made based upon how much the amount of the lock is fees from other locks using the same underlying pool will also be claimed and sent to the owner.

This may simply be unexpected for the owner using the locker contract and also needs to be accounted for in any integrating protocol using Ullr as a locking partner to avoid fees being stolen by other users or a DoS of the minFeeAmounts validation.

Let this finding document this potentially unexpected behavior for users and integrating protocols.

In the NatSpec for the topUpLock function it is mentioned that the validation ensures that the ownerCannot top up locks where withdrawal is possible to prevent misleading users.

However the topUpLock function can still be called when the user’s lock duration has passed, but they have not yet triggered their withdrawal. The withdrawal delay assigned for the lock can be a very short period, a number of seconds, and allow the user to topUpLock and triggerWithdrawal to withdraw very shortly afterwards.

Users can still verify the state of the lock and withdrawal delay on-chain, consider if the current behavior is acceptable or if the validation should be changed to ensure that the users lock is not expired.

Some ERC20 tokens such as cUSDCv3 have the behavior that transferring the maximum type(uint256).max value transfers the entire account balance instead.

This may create unexpected issues depending on how protocols use the Ullrlocker, where the user specifies type(uint256).max as the amount and the from address only transfers a small amount of tokens. A malicious actor can create a targeted overflow using this method in the topUpLock function which allows them to assign the amount of their lock to the entire token balance of the contract and steal tokens which were not theirs.

Consider using the token amount which was received in the topUpLock and lockTokens functions instead of the amount specified by the user.

Otherwise be aware and document the fact that the locker system should be incompatible with such tokens.

The upgradeImplementation function validates that the withdrawalAllowedAt is not greater than zero to prevent users who have triggered withdrawals from upgrading.

This bears some risk in that users who have a long withdrawal delay cannot upgrade their underlying protocol implementation contract in case a security issue is discovered in it.

Be aware of this risk and consider if it is desired.

Throughout the UllrLocker contract the _locks mapping is directly accessed, however the _getLockInfo function already exists to access the _locks mapping in a safe way with the appropriate empty struct check.

Consider constricting access of the _locks mapping directly to just the _getLockInfo function and using the _getLockInfo to access the lock where necessary in the UllrLocker contract.

The computeLockerAddress function is not called inside the UllrFactory.sol contract, and can thus be made external to save some gas when calling it.

Set computeLockerAddress to external visibility.

The Ullr admin has the ability to enable emergency withdrawals for specific logic contracts. This is inflexible in the case where a specific LP or underlying token has an issue, the locker owner will not be able to withdraw it.

Adding an emergency withdraw function specific to the locker would mitigate issues where withdrawals cannot be completed due to issues specific to LP tokens or the underlying token transfers.

The lockTokensWithPermit and topUpLockWithPermit functions were added to save users from having to perform an approval transactions prior to locking tokens. However, these two functions do not verify the intent of the permit which can result in permanently frozen funds for any permits granted to this contract.

A malicious user can monitor the mempool and frontrun transactions to either of these functions. The attacker can supply a nearly infinite lock time, permanently freezing the funds in the contract.

Require another signature provided to both functions that verifies the intent of the action. Construct a permit type hash for each action and also hash the individual parameters of the new lock or top up, such as lock duration and withdrawal delay.

Verify that the signer of the hash is the from address.

H-01 was partially fixed by properly verifying that the LP token used is a legitimate Aerodrome LP token. However, there is still a vector for the owner to be able to withdraw their LP early.

The owner can use the Aerodrome pool factory to create a legitimate LP using another Aerodrome LP as token0 a custom owner-controlled ERC20 with hooks as token1.

In the same way outlined in H-01, when token1 is transferred through the fee claiming mechanism, control will be transferred to the owner's token contract which can then lock more LP (token0) with a 1 second lock.

Since the LP (token0) is transferred to the Locker contract during the fee claim, the Locker contract transfers all of the new LP (token0) to the owner. Then the owner can withdraw their new lock 1 second later, bypassing all locks in the process.

Steps:

• Owner locks LPX in Locker.sol
• Owner deploys MaliciousToken with transfer hook
• Owner creates LPY via the Aerodrome PoolFactory which takes LPX as token0 and MaliciousToken as token1
• Owner locks some LPY in Locker.sol
• Owner calls collectAndProcessFees on LPY. When MaliciousToken is transferred to the Locker contract, it will also call Locker.lockTokens() to lock LPX for only 1 second.
• The extra LPX deposited will be accounted for as claimed fees and sent to the owner.
• Owner withdraws the newest lock 1 second later.

Add reentrancy guards to all user-facing functions.

The interface used in the Ullr codebase for the permitTransferFromWithAdditionalDataERC20 function is as follows:

    function permitTransferFromWithAdditionalDataERC20(
        address token,
        uint256 nonce,
        uint256 permitAmount,
        uint256 expiration,
        address owner,
        address to,
        uint256 transferAmount,
        bytes memory signedPermit,
        bytes memory additionalData,
        bytes32 typeHash
    )
        external
        returns (bool isError);

However the interface that is available in the deployed PermitC contract is:

    function permitTransferFromWithAdditionalDataERC20(
        address token,
        uint256 nonce,
        uint256 permitAmount,
        uint256 expiration,
        address owner,
        address to,
        uint256 transferAmount,
        bytes32 additionalData,
        bytes32 advancedPermitHash,
        bytes calldata signedPermit
    ) external returns (bool isError);

As a result these interfaces do not match up with the PermitC contract available onchain.

Consider updating the interface used to match the PermitC contracts onchain.

The LOCK_TOKENS_TYPEHASH and TOP_UP_LOCK_TYPEHASH are missing the function name and opening parenthesis at the beginning of the string.

Include the relevant beginning for:

LOCK_TOKENS_TYPEHASH: keccak256("address token,uint256 amount,uint256 withdrawalDelay,uint256 lockPeriod,address implementation)”)

TOP_UP_LOCK_TYPEHASH: keccak("bytes32 lockId,uint256 amount)")

In the _processFees function if the token transfer provided no return data the success of the transfer is assumed to be true. However in the event that the target token contract does not house bytecode this will errantly indicate that a transfer has successfully occurred for a token that does not exist.

It is unlikely that this case would arise with the current underlying implementation, however future implementation contracts may allow this edge case to arise.

Validate that the token address houses bytecode in the _processFees function.

The following external functions lack nonReentrant modifiers:

• triggerWithdrawal
• cancelWithdrawalTrigger

Although no explicit exploit path has been identified with these functions, out of an abundance of caution you may consider adding nonReentrant modifiers to these functions as well.

Consider adding nonReentrant modifiers to these functions.

The collectFeesForToken function allows the caller to supply their own implementation contract which must be an active implementation which supports the token passed.

This allows for potentially unexpected edge cases where a locker owner uses a different implementation to claim their fees than the one that is actively associated with one or more ongoing locks for that token.

This same edge case also applies when a locker owner holds multiple locks for the same token with different implementation versions being used.

Simply be aware of this edge case and consider it for future iterations of the implementations available for lockers.