<div align="center">
<center>
<img src="https://avatars.githubusercontent.com/u/167952721" height="350" alt="@offbeatsecurity" style="margin-bottom: 20px;">
<h1>Liquiditypad Vault</h1>
<h3>February 24, 2025</h3>
<p>Prepared for GOAT Network</p>
<p>Conducted by:</p>
<p>Kurt Willis (phaze)</p>
<p>Richie Humphrey (devtooligan)</p>
</center>
</div>

## About the Project
The GOAT Network Liquiditypad Vault is a Layer 2 solution that enables secure asset bridging and liquidity provision between Bitcoin and EVM-compatible chains. Built on technologies like BitVM and zkMIPS, it implements an Optimistic Challenge Protocol (GOAT-OCP) for trust-minimized ZKP verification. The vault contract provides functionality for token deposits, withdrawals, and LP token management with configurable wait periods and whitelisting capabilities.

## About Offbeat Security
Offbeat Security is a boutique security company providing unique security solutions for complex and novel crypto projects. Our mission is to elevate the blockchain security landscape through invention and collaboration.

## Summary & Scope
The [src](https://github.com/GOATNetwork/liquiditypad-vault-contracts/tree/d6bf5a64f7ac268059d7871a2e683065b1a61644/) folder was reviewed at commit [d6bf5a6](https://github.com/GOATNetwork/liquiditypad-vault-contracts/tree/d6bf5a64f7ac268059d7871a2e683065b1a61644/).

The following 2 contracts were in scope:
- src/AssetVault.sol
- src/LPToken.sol

The protocol implements a bridging and liquidity provision mechanism through a vault contract that manages deposits, withdrawals, and LP token generation. The system uses Layer Zero for cross-chain messaging and includes configurable parameters for withdrawal wait periods and access control.

The review identified 1 HIGH, 6 LOW, and 2 INFO severity issues. Key findings include a high-severity vulnerability with canceling processed withdrawals leading to reward loss, unnecessary whitelist implementation increasing centralization risk, missing token validation, and timing issues with the withdrawal process. Additional recommendations focus on code quality improvements and standardization of terminology.

## Summary of Findings

| Identifier | Title | Severity | Fixed |
|------------|-------|----------|-------|
| [H-01](#H-01-Canceling-a-processed-withdrawal-request-results-in-loss-of-all-past-and-future-rewards) | Canceling a processed withdrawal request results in loss of all past and future rewards | High | Fixed in [5664a5c](https://github.com/GOATNetwork/liquiditypad-vault-contracts/commit/5664a5c038959ab9b9ee44089002e0c94ca8426b) |
| [L-01](#L-01-Missing-validation-for-Layer-Zero-message-fee-parameters) | Missing validation for Layer Zero message fee parameters | Low | Fixed in [071fee3](https://github.com/GOATNetwork/liquiditypad-vault-contracts/commit/071fee33cc88bd3efe767177f30ddb22606ddd98) |
| [L-02](#L-02-Unnecessary-whitelist-implementation-increases-centralization-risk) | Unnecessary whitelist implementation increases centralization risk | Low | Fixed in [201e7d2](https://github.com/GOATNetwork/liquiditypad-vault-contracts/commit/201e7d2ebe86cddd65fdd97d315fcdbf9c5f7d19) |
| [L-03](#L-03-Token-contract-validation-missing-in-addUnderlyingToken-function) | Token contract validation missing in addUnderlyingToken function | Low | Fixed in [205cb0b](https://github.com/GOATNetwork/liquiditypad-vault-contracts/commit/205cb0b9f7658d41b827baaa10cddd02e60709b9) |
| [L-04](#L-04-Wait-period-changes-affect-pending-withdrawals-with-unclear-documentation) | Wait period changes affect pending withdrawals with unclear documentation | Low | Fixed in [2f753c8](https://github.com/GOATNetwork/liquiditypad-vault-contracts/commit/2f753c82cfcf1d280bb8c54d94d0f54271bf38f1) |
| [L-05](#L-05-Cancelled-withdrawals-can-enable-early-processing-of-pending-requests) | Cancelled withdrawals can enable early processing of pending requests | Low | Fixed in [ded7d1c](https://github.com/GOATNetwork/liquiditypad-vault-contracts/commit/ded7d1c9ee7598a607dd23c4c807e6b869bfe6f6) |
| [L-06](#L-06-Function-naming-inconsistent-with-EIP-7540-terminology) | Function naming inconsistent with EIP-7540 terminology | Low | Fixed in [ccb4a68](https://github.com/GOATNetwork/liquiditypad-vault-contracts/commit/ccb4a6881bc4f2ffa67352984c06102fd4427b1b) |
| [I-01](#I-01-Missing-documentation-for-rebasing-token-limitation) | Missing documentation for rebasing token limitation | Info | Fixed in [b9ca40a](https://github.com/GOATNetwork/liquiditypad-vault-contracts/commit/b9ca40a51d2f0396b887a56906d16f203546f98a) |
| [I-02](#I-02-Missing-implementation-of-_disableInitializers-vulnerability) | Missing implementation of _disableInitializers() vulnerability | Info | Acknowledged |
| [Code quality](#Code-quality-improvements) | Code quality improvements | Info | Fixed in [552a7b3](https://github.com/GOATNetwork/liquiditypad-vault-contracts/commit/552a7b3713118aeb2e760779946c2c11d266c266), [944e0f3](https://github.com/GOATNetwork/liquiditypad-vault-contracts/commit/944e0f37710befe359502536ebe849202bc7d5c8), [9358178](https://github.com/GOATNetwork/liquiditypad-vault-contracts/commit/93581782a3d304ff374322ac330f13828f920fa2) |
| [Optimizations](#Gas-optimizations) | Gas optimizations | Info | Fixed in [01788c2](https://github.com/GOATNetwork/liquiditypad-vault-contracts/commit/01788c20db1674d0e36ace83fd3774bbd54e474c) |

## Detailed Findings

### High Findings

### [H-01] Canceling a processed withdrawal request results in loss of rewards

## Summary

The `cancelWithdrawal()` function only verifies if a withdrawal request has been completed, but fails to check if it has been processed. This allows users to cancel a processed withdrawal request, resulting in the cessation of future reward accrual without the user's knowledge.

## Description

In the `AssetVault` contract, the withdrawal process involves multiple steps:
1. A user calls `requestWithdraw()` to initiate the withdrawal
2. An admin processes withdrawal requests up to a certain ID using `processUntil()`
3. The user claims their processed withdrawal via `claim()`

The issue exists in the `cancelWithdrawal()` function:

function cancelWithdrawal(uint64 _id) external {
    WithdrawalRequest memory withdrawalRequest = withdrawalRequests[_id];
    require(!withdrawalRequest.isCompleted, "Already completed");
    address requester = withdrawalRequest.requester;
    require(msg.sender == requester, "Wrong requester");

    IToken(underlyingTokens[withdrawalRequest.requestToken].lpToken)
        .transfer(requester, withdrawalRequest.lpAmount);

    delete withdrawalRequests[_id];
    emit WithdrawalCancelled(
        requester,
        withdrawalRequest.requestToken,
        _id,
        withdrawalRequest.lpAmount
    );
}

This function only checks if the withdrawal request has been completed (`!withdrawalRequest.isCompleted`), but doesn't verify if it has been processed. A withdrawal is considered processed when its ID is less than or equal to `processedWithdrawalCounter`, which is updated by the `processUntil()` function.

If a user cancels a withdrawal after it has been processed but before claiming it, they would:
1. Receive back their LP tokens from the contract
2. Stop accruing future rewards without being aware of this change in status

Users might cancel and resubmit withdrawals for various reasons, unaware that the withdrawal was already processed and that their reward accrual has stopped. Since the contract doesn't provide any indication that reward accrual has ceased, users might continue to expect rewards while actually receiving none, leading to unexpected financial losses.

## Recommendation

Modify the `cancelWithdrawal()` function to prevent cancellation of processed withdrawal requests:

function cancelWithdrawal(uint64 _id) external {
    WithdrawalRequest memory withdrawalRequest = withdrawalRequests[_id];
    require(!withdrawalRequest.isCompleted, "Already completed");
+   require(_id > processedWithdrawalCounter, "Already processed");
    address requester = withdrawalRequest.requester;
    require(msg.sender == requester, "Wrong requester");

This additional check ensures users cannot cancel withdrawal requests that have already been processed, protecting them from unintentionally losing their accumulated rewards and future reward accrual.

### Low Findings

### [L-01] Missing validation for Layer Zero message fee parameters

#### Description
The `deposit()` function accepts a `MessagingFee` parameter but does not validate that the provided fee matches the requirements of the Layer Zero endpoint. Specifically, it should enforce that `lzTokenFee` is zero (since Layer Zero tokens are not used) and that `nativeFee` equals `msg.value` to ensure the correct fee is provided.

The current implementation could lead to user confusion or errors if they attempt to use the `lzTokenFee` parameter or provide a `nativeFee` that doesn't match the sent ETH value.

#### Recommendation
Add input validation for the provided `MessagingFee` parameter:

function deposit(
    address _token,
    uint256 _amount,
    MessagingFee memory _fee
) external payable nonReentrant {
    UnderlyingToken memory tokenInfo = underlyingTokens[_token];
    require(tokenInfo.lpToken != address(0), "Invalid token");
    require(!depositPaused[_token], "Deposit paused");
    require(_amount >= tokenInfo.minDepositAmount, "Invalid amount");
    require(
        !whitelistMode[_token] || depositWhitelist[_token][msg.sender],
        "Not whitelisted"
    );
+   require(_fee.lzTokenFee == 0, "LZ token fee not supported");
+   require(_fee.nativeFee == msg.value, "Invalid native fee");
    IToken(_token).transferFrom(msg.sender, address(this), _amount);
    IOFT(tokenInfo.bridge).send{value: msg.value}(
        generateSendParam(_amount),
        _fee,
        msg.sender
    );

### [L-02] Unnecessary whitelist implementation increases centralization risk

## Description

The `LPToken` contract implements a mechanism to disable token transfers using a `transferSwitch` boolean, with an exception for whitelisted addresses. The primary use case appears to be allowing only the vault contract to call `transfer()` and `transferFrom()` while transfers are paused for regular users.

Currently, this exception is implemented using a mapping:

mapping(address => bool) public transferWhitelist;

This implementation unnecessarily increases centralization risk. If an EOA (externally owned account) were to be added to the whitelist, it could bypass transfer restrictions for any token holder who has approved it. For instance, if a user previously approved this EOA for token management (a common pattern in DeFi), the whitelisted EOA could transfer tokens on the user's behalf even during a transfer freeze period.

This creates a potential centralization vector where a few whitelisted addresses gain significant control over token movement during restricted periods, beyond what may be necessary for the intended vault functionality.

The current implementation allows any whitelisted address to transfer tokens on behalf of any user who has approved them:

function transferFrom(
    address from,
    address to,
    uint256 value
) public override returns (bool) {
    require(
        transferSwitch || transferWhitelist[msg.sender],
        "LPToken: transfer is paused"
    );
    return super.transferFrom(from, to, value);
}

## Recommendation

To reduce centralization risk while maintaining necessary functionality, consider replacing the whitelist mapping with a single address variable to limit exceptional transfer abilities to only the vault contract:

- mapping(address => bool) public transferWhitelist;
+ address public vaultAddress;

function setTransferWhitelist(
    address _address,
    bool _isWhitelisted
) external onlyRole(DEFAULT_ADMIN_ROLE) {
-   transferWhitelist[_address] = _isWhitelisted;
-   emit SetTransferWhitelist(_address, _isWhitelisted);
+   vaultAddress = _isWhitelisted ? _address : address(0);
+   emit SetVaultAddress(_address, _isWhitelisted);
}

function transfer(
    address to,
    uint256 value
) public override returns (bool) {
    require(
-       transferSwitch || transferWhitelist[msg.sender],
+       transferSwitch || msg.sender == vaultAddress,
        "LPToken: transfer is paused"
    );
    return super.transfer(to, value);
}

function transferFrom(
    address from,
    address to,
    uint256 value
) public override returns (bool) {
    require(
-       transferSwitch || transferWhitelist[msg.sender],
+       transferSwitch || msg.sender == vaultAddress,
        "LPToken: transfer is paused"
    );
    return super.transferFrom(from, to, value);
}

### [L-03] Token contract validation missing in addUnderlyingToken function

#### Description
The `addUnderlyingToken()` function does not validate that the provided `_lpToken` address is a valid ERC20 token contract. If an invalid address is provided, the token cannot be removed later since `removeUnderlyingToken()` will revert when trying to call `balanceOf()` on the invalid contract.

Currently, the only validation performed on `_lpToken` is checking if it's already been registered:

require(
    lpToUnderlyingTokens[_lpToken] == address(0),
    "Existing LP token"
);

This issue could result in tokens being permanently stuck in the protocol if an administrator accidentally adds a token with an invalid LP token address, as they would be unable to remove it later.

#### Recommendation
Add validation in `addUnderlyingToken()` to verify that the provided `_lpToken` implements the required ERC20 interface:

function addUnderlyingToken(
    address _token,
    address _lpToken,
    address _bridge,
    uint256 _minDepositAmount,
    uint256 _minWithdrawAmount
) external onlyRole(ADMIN_ROLE) {
    require(_token != address(0), "Invalid token");
+   require(_lpToken != address(0), "Invalid LP token address");
    require(
        lpToUnderlyingTokens[_lpToken] == address(0),
        "Existing LP token"
    );
    require(underlyingTokens[_token].lpToken == address(0), "Token exists");
    uint8 decimals = IToken(_token).decimals();
    require(decimals <= 18, "Invalid decimals");
+   // Verify LP token implements required interface
+   try IToken(_lpToken).balanceOf(address(this)) returns (uint256) {
+       // Success - token implements balanceOf
+   } catch {
+       revert("Invalid LP token");    
+   }

### [L-04] Wait period changes affect pending withdrawals with unclear documentation

#### Description
The `setRedeemWaitPeriod()` function can modify the wait period for withdrawals, but this change affects all existing pending withdrawals since the absolute unlock time is not stored with each withdrawal request. Instead, `processUntil()` checks the wait period dynamically:

require(
    withdrawalRequests[_id].timestamp + redeemWaitPeriod <= block.timestamp,
    "Time not reached"
);

This means that changing `redeemWaitPeriod` can unexpectedly extend or shorten the wait time for existing requests. The current sequential processing design in `processUntil()` assumes that unlock times always increase with ID, which makes this behavior particularly important to document.

#### Recommendation
Add clear documentation to the relevant functions to make this behavior explicit:

+   /// @notice Sets the waiting period for withdrawals
+   /// @dev This change affects ALL pending withdrawals, not just new ones. 
+   /// Increasing the wait period will delay processing of existing requests.
+   /// Decreasing it will allow earlier processing.
+   /// @param _redeemWaitPeriod New waiting period in seconds
    function setRedeemWaitPeriod(
        uint32 _redeemWaitPeriod
    ) external onlyRole(ADMIN_ROLE) {
        redeemWaitPeriod = _redeemWaitPeriod;
        emit SetRedeemWaitPeriod(_redeemWaitPeriod);
    }

+   /// @notice Process withdrawal requests up to the specified ID
+   /// @dev Uses current redeemWaitPeriod value to check if requests can be processed.
+   /// Changes to redeemWaitPeriod affect when existing requests become processable.
+   /// @param _id The withdrawal request ID up to which to process
    function processUntil(uint64 _id) external onlyRole(ADMIN_ROLE) {

### [L-05] Cancelled withdrawals can enable early processing of pending requests

#### Description
The `processUntil()` function uses the timestamp of a specified withdrawal request to determine if enough time has passed to process all requests up to that ID. However, it doesn't verify that the withdrawal request still exists, as cancelled requests have their data deleted:

// In cancelWithdrawal()
delete withdrawalRequests[_id];

// In processUntil()
require(
    withdrawalRequests[_id].timestamp + redeemWaitPeriod <= block.timestamp,
    "Time not reached"
);

If a withdrawal request is cancelled, its timestamp becomes 0. An admin could specify this cancelled ID in `processUntil()`, allowing all previous pending requests to be processed even if their wait period hasn't elapsed, since `0 + redeemWaitPeriod` would be less than the current timestamp.

#### Recommendation
Add validation to ensure the withdrawal request exists and hasn't been cancelled:

function processUntil(uint64 _id) external onlyRole(ADMIN*ROLE) {
    require(
        _id > processedWithdrawalCounter && _id < withdrawalCounter,
        "Invalid id"
    );
+   require(withdrawalRequests[_id].timestamp != 0, "Request cancelled");
    require(
        withdrawalRequests[_id].timestamp + redeemWaitPeriod <= block.timestamp,
        "Time not reached"
    );
    processedWithdrawalCounter = _id;
    emit WithdrawalProcessed(_id);
}

### [L-06] Function naming inconsistent with EIP-7540 terminology

#### Description
The contract uses inconsistent terminology that doesn't align with EIP-7540 specifications. The current implementation uses "withdraw" terminology when the functions are actually performing redemption operations (converting shares to assets):

function requestWithdraw(
    address _requestToken,
    address _receiver,
    uint256 _lpAmount
) external returns (uint256 id) {
    // ...
}

function cancelWithdrawal(uint64 _id) external {
    // ...
}

struct WithdrawalRequest {
    bool isCompleted;
    address requester;
    address receiver;
    address requestToken;
    uint32 timestamp;
    uint256 lpAmount;  // represents shares
}

This terminology is misleading since the operations deal with share amounts (LP tokens) rather than asset amounts, making them redemption operations by definition.

#### Recommendation
Update function and variable names to align with EIP-7540 terminology:

- function requestWithdraw(
+ function requestRedeem(
    address _requestToken,
    address _receiver,
-   uint256 _lpAmount
+   uint256 _shares
) external returns (uint256 id) {
    // ...
}

- function cancelWithdrawal(
+ function cancelRedeem(
    uint64 _id
) external {
    // ...
}

- struct WithdrawalRequest {
+ struct RedeemRequest {
    bool isCompleted;
    address requester;
    address receiver;
    address requestToken;
    uint32 timestamp;
-   uint256 lpAmount;
+   uint256 shares;
}

- event WithdrawalRequested(
+ event RedeemRequested(
    address indexed requester,
    address indexed receiver,
    address indexed requestToken,
    uint256 id,
-   uint256 lpAmount
+   uint256 shares
);

### Informational Findings

### [I-01] Missing documentation for rebasing token limitation

## Description

The AssetVault contract does not handle rebasing tokens correctly as it maintains a fixed LP token amount relative to the initial deposit amount, without accounting for balance changes from rebasing events. If used with rebasing tokens, this could lead to a mismatch between LP token supply and actual underlying asset balances.

When a rebasing event occurs:
1. The underlying token balance in the vault changes
2. The corresponding LP token amount remains unchanged
3. This creates an imbalance between asset backing and LP token value

## Recommendation

Add clear documentation about this limitation in the contract:

// SPDX-License-Identifier: MIT
pragma solidity 0.8.27;

+ /**
+  * @title AssetVault
+  * @notice A vault contract for bridging assets to the Goat network
+  * @dev This contract is not compatible with rebasing tokens. Using rebasing tokens
+  * will result in incorrect LP token ratios as the contract does not adjust LP token
+  * balances when underlying token balances change due to rebasing events.
+  */
contract AssetVault is AccessControlUpgradeable, ReentrancyGuardUpgradeable {

Consider also adding a check in `addUnderlyingToken()` that emits a warning event if the token appears to be rebasing.

### [I-02] Missing implementation of _disableInitializers() vulnerability

## Description

The `AssetVault` contract inherits from upgradeable contracts but does not implement the `_disableInitializers()` function in its constructor. This omission leaves the implementation contract vulnerable to potential initialization.

In upgradeable contracts, the implementation contract should have its initializers disabled to prevent it from being initialized directly. While the proxy contract delegates calls to the implementation, the implementation contract itself should not be initializable as a standalone contract. OpenZeppelin's upgradeable contracts provide a `_disableInitializers()` function that should be called in the constructor to ensure the implementation contract cannot be initialized.

Current constructor implementation:

constructor(uint32 _eid) {
    eid = _eid;
}

## Recommendation

Add a call to `_disableInitializers()` in the constructor to prevent the implementation contract from being initialized:

constructor(uint32 _eid) {
    eid = _eid;
+   _disableInitializers();
}

This ensures that only the proxy contract can be initialized, while the implementation contract remains in an uninitialized state, preventing potential security issues in an upgradeable contract system.

## Additional Recommendations

### Code quality improvements

1. **Improve function naming consistency**

Rename functions to better reflect their state-setting nature:

- function setDepositPause(
+ function setDepositPaused(
    address _token,
    bool _pause
) external onlyRole(ADMIN_ROLE)

- function setWithdrawPause(
+ function setWithdrawPaused(
    address _token,
    bool _pause
) external onlyRole(ADMIN_ROLE)

- event WithdrawalProcessed(
+ event WithdrawalsProcessed(
    uint256 id
);

2. **Add comprehensive NatSpec documentation**

Add detailed documentation for public functions:

/// @notice Sets whether deposits are paused for a specific token
/// @param _token The token address to set pause state for
/// @param _pause True to pause deposits, false to unpause
/// @dev Only callable by admin role
function setDepositPaused(address _token, bool _pause) external onlyRole(ADMIN_ROLE)

/// @notice Processes all withdrawal requests up to the specified ID
/// @param _id The ID up to which to process withdrawals
/// @dev All withdrawals up to this ID must have passed the wait period
function processUntil(uint64 _id) external onlyRole(ADMIN_ROLE)

3. **Initialize redeemWaitPeriod in constructor**

Add wait period to initialization:

 function initialize(
-    address _goatSafeAddress
+    address _goatSafeAddress,
+    uint32 _redeemWaitPeriod
 ) public initializer {
     __AccessControl_init();
     __ReentrancyGuard_init();
     _grantRole(DEFAULT_ADMIN_ROLE, msg.sender);
     goatSafeAddress = bytes32(uint256(uint160(_goatSafeAddress)));
+    redeemWaitPeriod = _redeemWaitPeriod;
     withdrawalCounter = 1;
 }

4. **Add maximum value check for redeemWaitPeriod**

 function setRedeemWaitPeriod(uint32 _redeemWaitPeriod) external onlyRole(ADMIN_ROLE) {
+    // Max wait period of 30 days
+    require(_redeemWaitPeriod <= 30 days, "Wait period too long");
     redeemWaitPeriod = _redeemWaitPeriod;
     emit SetRedeemWaitPeriod(_redeemWaitPeriod);
 }

5. **Replace array iteration with EnumerableSet**

Replace unbounded array iteration with OpenZeppelin's EnumerableSet:

+ import "@openzeppelin/contracts/utils/structs/EnumerableSet.sol";

 contract AssetVault is AccessControlUpgradeable, ReentrancyGuardUpgradeable {
+    using EnumerableSet for EnumerableSet.AddressSet;
-    address[] public underlyingTokenList;
+    EnumerableSet.AddressSet private underlyingTokenSet;

     function addUnderlyingToken(...) external onlyRole(ADMIN_ROLE) {
-        underlyingTokenList.push(_token);
+        underlyingTokenSet.add(_token);
     }

     function removeUnderlyingToken(address _token) external onlyRole(ADMIN_ROLE) {
-        // Remove from array loop
-        address[] memory tokens = underlyingTokenList;
-        uint256 length = tokens.length;
-        for (uint8 i; i < length; i++) {
-            if (tokens[i] == _token) {
-                underlyingTokenList[i] = underlyingTokenList[length - 1];
-                underlyingTokenList.pop();
-                break;
-            }
-        }
+        underlyingTokenSet.remove(_token);
     }

     function getUnderlyings() external view returns (address[] memory) {
-        return underlyingTokenList;
+        return underlyingTokenSet.values();
     }
 }

## Gas optimizations

1. **Replace uint8 loop counter with uint256**

Using uint8 for loop counters is less efficient due to additional required conversions:

function removeUnderlyingToken(address _token) external onlyRole(ADMIN_ROLE) {
    address[] memory tokens = underlyingTokenList;
    uint256 length = tokens.length;
-   for (uint8 i; i < length; i++) {
+   for (uint256 i; i < length; i++) {
        if (tokens[i] == _token) {
            underlyingTokenList[i] = underlyingTokenList[length - 1];
            underlyingTokenList.pop();
            break;
        }
    }
}

2. **Use uint256 for withdrawal IDs**

Replace uint64 with uint256 to improve efficiency and remove protocol limitations:

-   uint64 public withdrawalCounter;
-   uint64 public processedWithdrawalCounter;
+   uint256 public withdrawalCounter;
+   uint256 public processedWithdrawalCounter;

-   function processUntil(uint64 _id) external onlyRole(ADMIN_ROLE) {
+   function processUntil(uint256 _id) external onlyRole(ADMIN_ROLE) {

-   function cancelWithdrawal(uint64 _id) external {
+   function cancelWithdrawal(uint256 _id) external {

-   function claim(uint64 _id) external nonReentrant {
+   function claim(uint256 _id) external nonReentrant {

3. **Increase timestamp size to uint48**

Update timestamp in WithdrawalRequest to uint48 for better timestamp range while maintaining storage packing:

struct WithdrawalRequest {
    bool isCompleted;
    address requester;
    address receiver;
    address requestToken;
-   uint32 timestamp;
+   uint48 timestamp;
    uint256 lpAmount;
}

This change provides ~280,000 years of timestamp range instead of ~136 years, without increasing gas costs since the struct remains optimally packed.

4. **Incorrect parameter name**

`LPToken.burn` has an incorrectly named parameter `_to`, should be `_from`.

    function burn(address _to, uint256 _amount) external onlyRole(MINT_ROLE) {
        _burn(_to, _amount);
    }