LayerZero Wrapped Asset Bridge v2 Security Review Report

In the WOFT.sol contract, the function _debit() calculates and deducts fees when burning tokens. This involves calling the _debitView() function, which determines the amount to be received after applying the fee and dust removal.

function _debit(
    address _from,
    uint256 _amountLD,
    uint256 _minAmountLD,
    uint32 _dstEid
) internal virtual override returns (uint256 amountSentLD, uint256 amountReceivedLD) {
    (amountSentLD, amountReceivedLD) = _debitView(_amountLD, _minAmountLD, _dstEid);
 
    uint256 fee = amountSentLD - amountReceivedLD;
    if (fee > 0) {
        _transfer(_from, address(WOFT_FACTORY), fee);
    }
    _burn(_from, amountReceivedLD);
}

However, the current logic for fee calculation in _debitView() inadvertently causes the fee transferred to the WOFT factory to be larger than intended. This discrepancy arises due to the sequence of operations. The function first assigns amountSentLD = _amountLD, then calculates amountReceivedLD = _removeDust(_amountLD - feeLD). This leads to the difference, uint256 fee = amountSentLD - amountReceivedLD, being greater than the initially calculated feeLD, resulting in users being overcharged.

Let's assume:

• LOCAL_DECIMALS = 18
• SHARED_DECIMALS = 6
• decimalConversionRate = 10^12 (10^(18-6))
• Fee rate = 0.1% (for simplicity) Example transaction: User wants to send 1.234567890123456789 tokens (1234567890123456789 in wei)

Step by step:

1. Initial _amountLD = 1234567890123456789
2. Calculate fee: feeLD = 0.1% of 1234567890123456789 = 1234567890123456 (rounded down)
3. Subtract fee: 1234567890123456789 - 1234567890123456 = 1233333322233333333
4. Apply _removeDust: 1233333322233333333 / 10^12 * 10^12 = 1233333322233000000
5. Set amountReceivedLD = 1233333322233000000
6. In the current implementation: amountSentLD = 1234567890123456789 (original amount) Actual fee transferred = 1234567890123456789 - 1233333322233000000 = 1234567890456789 We can see that the actual fee transferred (1234567890456789) is larger than the calculated fee (1234567890123456) by 333333 wei.

function _debitView(
    uint256 _amountLD,
    uint256 _minAmountLD,
    uint32 /*_dstEid*/
) internal view override returns (uint256 amountSentLD, uint256 amountReceivedLD) {
    amountSentLD = _amountLD;
 
    // @dev Check for fees on the factory
    uint256 feeLD = WOFT_FACTORY.getFee(address(this), _amountLD);
    amountReceivedLD = _removeDust(_amountLD - feeLD);
 
    // @dev Check for slippage
    if (amountReceivedLD < _minAmountLD) revert SlippageExceeded(amountReceivedLD, _minAmountLD);
}

The fee should be calculated after dust removal to ensure the correct fee amount is transferred:

function _debitView(
    uint256 _amountLD,
    uint256 _minAmountLD,
    uint32 /*_dstEid*/
) internal view override returns (uint256 amountSentLD, uint256 amountReceivedLD) {
--  amountSentLD = _amountLD;
 
    // @dev Check for fees on the factory
    uint256 feeLD = WOFT_FACTORY.getFee(address(this), _amountLD);
    amountReceivedLD = _removeDust(_amountLD - feeLD);
 
++  amountSentLD = amountReceivedLD + feeLD;
 
    // @dev Check for slippage
    if (amountReceivedLD < _minAmountLD) revert SlippageExceeded(amountReceivedLD, _minAmountLD);
}

In the WOFTFactory contract's constructor, _defaultFeeBps must be less than BPS_DENOMINATOR. However, in the WOFTFactory.setDefaultFeeBps() function, the feeBps can be equal to BPS_DENOMINATOR, resulting in an inconsistency in the fee boundary requirements of the two functions.

function setDefaultFeeBps(uint16 _feeBps) external onlyOwner {
    if (_feeBps > BPS_DENOMINATOR) revert InvalidBps();

constructor(
    address _endpoint,
    address _delegate,
    uint16 _defaultFeeBps
) OApp(_endpoint, _delegate) Ownable(_delegate) {
    if (_defaultFeeBps >= BPS_DENOMINATOR) revert InvalidBps();

Consider modifying the WOFTFactory's constructor as follows:

constructor(
    address _endpoint,
    address _delegate,
    uint16 _defaultFeeBps
) OApp(_endpoint, _delegate) Ownable(_delegate) {
--      if (_defaultFeeBps >= BPS_DENOMINATOR) revert InvalidBps();
++      if (_defaultFeeBps > BPS_DENOMINATOR) revert InvalidBps();
    defaultFeeBps = _defaultFeeBps;
}

Mentioned contracts import OZ's Ownable.sol, as the contracts are non-upgradeable and have some essential onlyOwner functionality, it is especially important that transfers of ownership should be handled with care.

Single-step ownership transfers add the risk of setting an unwanted owner by accident if the ownership transfer is not done with excessive care.

import { Ownable } from "@openzeppelin/contracts/access/Ownable.sol";

Use OpenZeppelin's Ownable2Step.sol.

The project uses public visibility for the most constant parameters in various contracts.

Setting these constants to private will save deployment gas. This is because the compiler won't have to create non-payable getter functions for deployment calldata, won't need to store the bytes of the values outside of where it's used, and won't add another entry to the method ID table. The values can still be read from the verified contract source code.

uint16 public constant SEND = 1;
uint16 public constant SEND_AND_CALL = 2;

address public immutable FACTORY;

uint32 public constant ADAPTER_EID = 30101;
uint256 public constant BPS_DENOMINATOR = 10_000;
address public constant NATIVE_TOKEN_ADDRESS = address(0xEeeeeEeeeEeEeeEeEeEeeEEEeeeeEeeeeeeeEEeE);

IWOFTFactory public immutable WOFT_FACTORY;

Make the constants and the immutable variables private instead of public.