Merge pull request #1617 from LayerZero-Labs/ovault-audit/paladin

shankars99 · shankars99 · commit c99fc923cc2b · 2025-07-20T19:04:37.000Z
feat: audit feedback for ovault (#1617)
diff --git a/examples/ovault-evm/test/mocks/MockOVault.sol b/examples/ovault-evm/test/mocks/MockOVault.sol
@@ -28,9 +28,8 @@ contract MockOVaultUpgradeable is OVaultUpgradeable {
         _disableInitializers();
     }
 
-    function __OVault_init(string memory _name, string memory _symbol, address _asset) internal onlyInitializing {
-        __ERC4626_init(IERC20(_asset));
-        __ERC20_init(_name, _symbol);
+    function initialize(string memory _name, string memory _symbol, address _asset) internal onlyInitializing {
+        __OVault_init(_name, _symbol, _asset);
     }
 
     function totalAssets() public view override returns (uint256) {
diff --git a/packages/ovault-evm/contracts/OVault.sol b/packages/ovault-evm/contracts/OVault.sol
@@ -5,6 +5,46 @@ import { IERC20 } from "@openzeppelin/contracts/token/ERC20/IERC20.sol";
 import { ERC20 } from "@openzeppelin/contracts/token/ERC20/ERC20.sol";
 import { ERC4626 } from "@openzeppelin/contracts/token/ERC20/extensions/ERC4626.sol";
 
+/**
+ * @notice From OpenZeppelin:
+
+ * @dev Implementation of the ERC-4626 "Tokenized Vault Standard" as defined in
+ * https://eips.ethereum.org/EIPS/eip-4626[ERC-4626].
+ *
+ * This extension allows the minting and burning of "shares" (represented using the ERC-20 inheritance) in exchange for
+ * underlying "assets" through standardized {deposit}, {mint}, {redeem} and {burn} workflows. This contract extends
+ * the ERC-20 standard. Any additional extensions included along it would affect the "shares" token represented by this
+ * contract and not the "assets" token which is an independent contract.
+ *
+ * [CAUTION]
+ * ====
+ * In empty (or nearly empty) ERC-4626 vaults, deposits are at high risk of being stolen through frontrunning
+ * with a "donation" to the vault that inflates the price of a share. This is variously known as a donation or inflation
+ * attack and is essentially a problem of slippage. Vault deployers can protect against this attack by making an initial
+ * deposit of a non-trivial amount of the asset, such that price manipulation becomes infeasible. Withdrawals may
+ * similarly be affected by slippage. Users can protect against this attack as well as unexpected slippage in general by
+ * verifying the amount received is as expected, using a wrapper that performs these checks such as
+ * https://github.com/fei-protocol/ERC4626#erc4626router-and-base[ERC4626Router].
+ *
+ * Since v4.9, this implementation introduces configurable virtual assets and shares to help developers mitigate that risk.
+ * The `_decimalsOffset()` corresponds to an offset in the decimal representation between the underlying asset's decimals
+ * and the vault decimals. This offset also determines the rate of virtual shares to virtual assets in the vault, which
+ * itself determines the initial exchange rate. While not fully preventing the attack, analysis shows that the default
+ * offset (0) makes it non-profitable even if an attacker is able to capture value from multiple user deposits, as a result
+ * of the value being captured by the virtual shares (out of the attacker's donation) matching the attacker's expected gains.
+ * With a larger offset, the attack becomes orders of magnitude more expensive than it is profitable. More details about the
+ * underlying math can be found xref:erc4626.adoc#inflation-attack[here].
+ *
+ * The drawback of this approach is that the virtual shares do capture (a very small) part of the value being accrued
+ * to the vault. Also, if the vault experiences losses, the users try to exit the vault, the virtual shares and assets
+ * will cause the first user to exit to experience reduced losses in detriment to the last users that will experience
+ * bigger losses. Developers willing to revert back to the pre-v4.9 behavior just need to override the
+ * `_convertToShares` and `_convertToAssets` functions.
+ *
+ * To learn more, check out our xref:ROOT:erc4626.adoc[ERC-4626 guide].
+ * ====
+ */
+
 contract OVault is ERC4626 {
     constructor(
         string memory _name,
diff --git a/packages/ovault-evm/contracts/OVaultComposer.sol b/packages/ovault-evm/contracts/OVaultComposer.sol
@@ -4,7 +4,6 @@ pragma solidity ^0.8.22;
 import { SafeERC20 } from "@openzeppelin/contracts/token/ERC20/utils/SafeERC20.sol";
 import { IERC20 } from "@openzeppelin/contracts/token/ERC20/utils/SafeERC20.sol";
 import { IERC4626 } from "@openzeppelin/contracts/interfaces/IERC4626.sol";
-import { SafeERC20 } from "@openzeppelin/contracts/token/ERC20/utils/SafeERC20.sol";
 
 import { ReentrancyGuard } from "@openzeppelin/contracts/utils/ReentrancyGuard.sol";
 
@@ -20,7 +19,6 @@ contract OVaultComposer is IOVaultComposer, ReentrancyGuard {
     using OFTComposeMsgCodec for bytes;
     using OFTComposeMsgCodec for bytes32;
     using SafeERC20 for IERC20;
-    using SafeERC20 for IERC20;
 
     address public immutable ASSET_OFT; // any OFT
     address public immutable SHARE_OFT; // lockbox adapter
@@ -68,9 +66,6 @@ contract OVaultComposer is IOVaultComposer, ReentrancyGuard {
         // Approve the shareOFTAdapter with the share tokens held by this contract
         IERC20(IOFT(_shareOFT).token()).approve(_shareOFT, type(uint256).max);
 
-        ASSET_DECIMAL_CONVERSION_RATE = IOFT(_assetOFT).decimalConversionRate();
-        SHARE_DECIMAL_CONVERSION_RATE = IOFT(_shareOFT).decimalConversionRate();
-
         REFUND_OVERPAY_ADDRESS = _refundOverpayAddress;
         ASSET_ERC20 = address(OVAULT.asset());
     }
@@ -126,9 +121,9 @@ contract OVaultComposer is IOVaultComposer, ReentrancyGuard {
         }
 
         /// @dev Try to execute the action on the target OFT. If we hit an error then it rolls back the storage changes.
-        try this.executeOVaultActionWithSlippageCheck(_refundOFT, amount, sendParam.minAmountLD) returns (
-            uint256 vaultAmount
-        ) {
+        try
+            this.executeOVaultActionWithSlippageCheck(_refundOFT, sendParam.dstEid, amount, sendParam.minAmountLD)
+        returns (uint256 vaultAmount) {
             /// @dev Setting the target amount to the actual value of the action (i.e. deposit or redeem)
             sendParam.amountLD = vaultAmount;
         } catch (bytes memory errMsg) {
@@ -156,8 +151,15 @@ contract OVaultComposer is IOVaultComposer, ReentrancyGuard {
         SendParam memory _sendParam,
         address _refundAddress
     ) external payable nonReentrant {
+        if (_sendParam.dstEid == HUB_EID && msg.value > 0) revert NoMsgValueOnSameChainOVaultAction();
+
         IERC20(ASSET_ERC20).safeTransferFrom(msg.sender, address(this), assetAmountLD);
-        _sendParam.amountLD = _executeOVaultActionWithSlippageCheck(ASSET_OFT, assetAmountLD, _sendParam.minAmountLD);
+        _sendParam.amountLD = _executeOVaultActionWithSlippageCheck(
+            ASSET_OFT,
+            _sendParam.dstEid,
+            assetAmountLD,
+            _sendParam.minAmountLD
+        );
         _send(SHARE_OFT, _sendParam, msg.value, _refundAddress);
     }
 
@@ -166,8 +168,15 @@ contract OVaultComposer is IOVaultComposer, ReentrancyGuard {
         SendParam memory _sendParam,
         address _refundAddress
     ) external payable nonReentrant {
+        if (_sendParam.dstEid == HUB_EID && msg.value > 0) revert NoMsgValueOnSameChainOVaultAction();
+
         IERC20(OVAULT).safeTransferFrom(msg.sender, address(this), shareAmountLD);
-        _sendParam.amountLD = _executeOVaultActionWithSlippageCheck(SHARE_OFT, shareAmountLD, _sendParam.minAmountLD);
+        _sendParam.amountLD = _executeOVaultActionWithSlippageCheck(
+            SHARE_OFT,
+            _sendParam.dstEid,
+            shareAmountLD,
+            _sendParam.minAmountLD
+        );
         _send(ASSET_OFT, _sendParam, msg.value, _refundAddress);
     }
 
@@ -195,12 +204,13 @@ contract OVaultComposer is IOVaultComposer, ReentrancyGuard {
     /// @dev External call for try...catch logic in lzCompose()
     function executeOVaultActionWithSlippageCheck(
         address _refundOFT,
+        uint32 _dstEid,
         uint256 _amount,
         uint256 _minAmountLD
     ) external nonReentrant returns (uint256 vaultAmount) {
         if (msg.sender != address(this)) revert OnlySelf(msg.sender);
 
-        vaultAmount = _executeOVaultActionWithSlippageCheck(_refundOFT, _amount, _minAmountLD);
+        vaultAmount = _executeOVaultActionWithSlippageCheck(_refundOFT, _dstEid, _amount, _minAmountLD);
     }
 
     /// @dev External call for try...catch logic in lzCompose()
@@ -212,7 +222,7 @@ contract OVaultComposer is IOVaultComposer, ReentrancyGuard {
             address _receiver = _sendParam.to.bytes32ToAddress();
             uint256 _amountLD = _sendParam.amountLD;
             IERC20 token = IERC20(IOFT(_oft).token());
-            token.transfer(_receiver, _amountLD);
+            token.safeTransfer(_receiver, _amountLD);
             if (msg.value > 0) {
                 (bool sent, ) = _receiver.call{ value: msg.value }("");
                 require(sent, "Failed to send Ether");
@@ -249,7 +259,6 @@ contract OVaultComposer is IOVaultComposer, ReentrancyGuard {
     function retry(bytes32 _guid, bool _removeExtraOptions) external payable nonReentrant {
         FailedMessage memory failedMessage = failedMessages[_guid];
         if (_failedGuidState(failedMessage) != FailedState.CanOnlyRetry) revert CanNotRetry(_guid);
-        if (_failedGuidState(failedMessage) != FailedState.CanOnlyRetry) revert CanNotRetry(_guid);
 
         SendParam memory sendParam = failedMessage.sendParam;
         uint256 prepaidMsgValue = failedMessage.msgValue;
@@ -277,6 +286,7 @@ contract OVaultComposer is IOVaultComposer, ReentrancyGuard {
 
         failedMessage.sendParam.amountLD = _executeOVaultActionWithSlippageCheck(
             failedMessage.refundOFT,
+            failedMessage.sendParam.dstEid, //dstEid
             failedMessage.refundSendParam.amountLD,
             failedMessage.sendParam.minAmountLD
         );
@@ -327,12 +337,13 @@ contract OVaultComposer is IOVaultComposer, ReentrancyGuard {
 
     function _executeOVaultActionWithSlippageCheck(
         address _refundOFT,
+        uint32 _dstEid,
         uint256 _amount,
         uint256 _minAmountLD
     ) internal returns (uint256) {
         (uint256 vaultAmount, address targetOFT) = _executeOVaultAction(_refundOFT, _amount);
 
-        _checkSlippage(targetOFT, vaultAmount, _minAmountLD);
+        _checkSlippage(targetOFT, _dstEid, vaultAmount, _minAmountLD);
 
         return vaultAmount;
     }
@@ -396,8 +407,10 @@ contract OVaultComposer is IOVaultComposer, ReentrancyGuard {
     /// @dev Remove dust before slippage check to be equivalent to OFTCore::_debitView()
     /// @dev If the OFT has a Fee or anything that changes the tokens such that:
     /// @dev dstChain.receivedAmount != srcChain.sentAmount, this will have to be overridden.
-    function _checkSlippage(address _oft, uint256 _amount, uint256 _minAmountLD) internal view virtual {
-        uint256 vaultAmountLD = _removeDust(_oft, _amount);
+    function _checkSlippage(address _oft, uint32 _dstEid, uint256 _amount, uint256 _minAmountLD) internal view virtual {
+        /// @dev In the event of a same chain deposit. Dust is never removed since we do not call OFT.send() but ERC20.transfer()
+        uint256 vaultAmountLD = HUB_EID == _dstEid ? _amount : _removeDust(_oft, _amount);
+
         uint256 amountReceivedLD = vaultAmountLD; /// @dev Perform your adjustments here if needed (ex: Fee)
         if (amountReceivedLD < _minAmountLD) {
             revert NotEnoughTargetTokens(amountReceivedLD, _minAmountLD);
diff --git a/packages/ovault-evm/contracts/OVaultUpgradeable.sol b/packages/ovault-evm/contracts/OVaultUpgradeable.sol
@@ -5,9 +5,54 @@ import { IERC20 } from "@openzeppelin/contracts/token/ERC20/IERC20.sol";
 import { ERC4626Upgradeable } from "@openzeppelin/contracts-upgradeable/token/ERC20/extensions/ERC4626Upgradeable.sol";
 import { Initializable } from "@openzeppelin/contracts-upgradeable/proxy/utils/Initializable.sol";
 
+/**
+ * @notice From OpenZeppelin:
+
+ * @dev Implementation of the ERC-4626 "Tokenized Vault Standard" as defined in
+ * https://eips.ethereum.org/EIPS/eip-4626[ERC-4626].
+ *
+ * This extension allows the minting and burning of "shares" (represented using the ERC-20 inheritance) in exchange for
+ * underlying "assets" through standardized {deposit}, {mint}, {redeem} and {burn} workflows. This contract extends
+ * the ERC-20 standard. Any additional extensions included along it would affect the "shares" token represented by this
+ * contract and not the "assets" token which is an independent contract.
+ *
+ * [CAUTION]
+ * ====
+ * In empty (or nearly empty) ERC-4626 vaults, deposits are at high risk of being stolen through frontrunning
+ * with a "donation" to the vault that inflates the price of a share. This is variously known as a donation or inflation
+ * attack and is essentially a problem of slippage. Vault deployers can protect against this attack by making an initial
+ * deposit of a non-trivial amount of the asset, such that price manipulation becomes infeasible. Withdrawals may
+ * similarly be affected by slippage. Users can protect against this attack as well as unexpected slippage in general by
+ * verifying the amount received is as expected, using a wrapper that performs these checks such as
+ * https://github.com/fei-protocol/ERC4626#erc4626router-and-base[ERC4626Router].
+ *
+ * Since v4.9, this implementation introduces configurable virtual assets and shares to help developers mitigate that risk.
+ * The `_decimalsOffset()` corresponds to an offset in the decimal representation between the underlying asset's decimals
+ * and the vault decimals. This offset also determines the rate of virtual shares to virtual assets in the vault, which
+ * itself determines the initial exchange rate. While not fully preventing the attack, analysis shows that the default
+ * offset (0) makes it non-profitable even if an attacker is able to capture value from multiple user deposits, as a result
+ * of the value being captured by the virtual shares (out of the attacker's donation) matching the attacker's expected gains.
+ * With a larger offset, the attack becomes orders of magnitude more expensive than it is profitable. More details about the
+ * underlying math can be found xref:erc4626.adoc#inflation-attack[here].
+ *
+ * The drawback of this approach is that the virtual shares do capture (a very small) part of the value being accrued
+ * to the vault. Also, if the vault experiences losses, the users try to exit the vault, the virtual shares and assets
+ * will cause the first user to exit to experience reduced losses in detriment to the last users that will experience
+ * bigger losses. Developers willing to revert back to the pre-v4.9 behavior just need to override the
+ * `_convertToShares` and `_convertToAssets` functions.
+ *
+ * To learn more, check out our xref:ROOT:erc4626.adoc[ERC-4626 guide].
+ * ====
+ */
 contract OVaultUpgradeable is ERC4626Upgradeable {
     /// @custom:oz-upgrades-unsafe-allow constructor
     constructor() {
         _disableInitializers();
     }
+
+    function __OVault_init(string memory _name, string memory _symbol, address _asset) public initializer {
+        __ERC4626_init(IERC20(_asset));
+        __ERC20_init(_name, _symbol);
+        __Context_init();
+    }
 }
diff --git a/packages/ovault-evm/contracts/interfaces/IOVaultComposer.sol b/packages/ovault-evm/contracts/interfaces/IOVaultComposer.sol
@@ -45,16 +45,14 @@ interface IOVaultComposer is IOAppComposer {
     error OnlyEndpoint(address caller);
     error OnlySelf(address caller);
     error OnlyOFT(address oft);
-    error OnlyAsset(address asset);
-    error OnlyShare(address share);
 
     error CanNotRefund(bytes32 guid);
     error CanNotRetry(bytes32 guid);
     error CanNotSwap(bytes32 guid);
-    error CanNotWithdraw(bytes32 guid);
 
     error NotEnoughTargetTokens(uint256 amountLD, uint256 minAmountLD);
     error NoMsgValueWhenSkippingRetry();
+    error NoMsgValueOnSameChainOVaultAction();
 
     /// ========================== GLOBAL VARIABLE FUNCTIONS =====================================
     function ASSET_OFT() external view returns (address);
diff --git a/packages/ovault-evm/test/composer/OVaultComposer_ProxySend.t.sol b/packages/ovault-evm/test/composer/OVaultComposer_ProxySend.t.sol
@@ -23,6 +23,17 @@ contract OVaultComposerProxySendTest is OVaultComposerBaseTest {
         vm.deal(userA, 100 ether);
     }
 
+    function test_target_is_hub_reverts_when_msg_value_provided() public {
+        SendParam memory sendParam = SendParam(ARB_EID, addressToBytes32(userA), TOKENS_TO_SEND, 0, "", "", "");
+        assetOFT_arb.mint(address(userA), TOKENS_TO_SEND);
+
+        vm.expectRevert(abi.encodeWithSelector(IOVaultComposer.NoMsgValueOnSameChainOVaultAction.selector));
+        OVaultComposerArb.depositSend{ value: 1 wei }(TOKENS_TO_SEND, sendParam, userA);
+
+        vm.expectRevert(abi.encodeWithSelector(IOVaultComposer.NoMsgValueOnSameChainOVaultAction.selector));
+        OVaultComposerArb.redeemSend{ value: 1 wei }(TOKENS_TO_SEND, sendParam, userA);
+    }
+
     function test_depositSend_target_hub() public {
         SendParam memory sendParam = SendParam(ARB_EID, addressToBytes32(userA), TOKENS_TO_SEND, 0, "", "", "");
         assetOFT_arb.mint(address(userA), TOKENS_TO_SEND);
@@ -36,6 +47,21 @@ contract OVaultComposerProxySendTest is OVaultComposerBaseTest {
         assertEq(assetOFT_arb.balanceOf(userA), 0);
     }
 
+    function test_depositSend_target_hub_can_have_dust() public {
+        uint256 amountWithDust = TOKENS_TO_SEND + 1; // 1 extra token to test dust handling
+
+        SendParam memory sendParam = SendParam(ARB_EID, addressToBytes32(userA), amountWithDust, 0, "", "", "");
+        assetOFT_arb.mint(address(userA), amountWithDust);
+
+        vm.startPrank(userA);
+        assetOFT_arb.approve(address(OVaultComposerArb), amountWithDust);
+        OVaultComposerArb.depositSend(amountWithDust, sendParam, userA);
+        vm.stopPrank();
+
+        assertEq(oVault_arb.balanceOf(userA), amountWithDust);
+        assertEq(assetOFT_arb.balanceOf(userA), 0);
+    }
+
     function test_redeemSend_target_hub() public {
         SendParam memory sendParam = SendParam(ARB_EID, addressToBytes32(userA), TOKENS_TO_SEND, 0, "", "", "");
         assetOFT_arb.mint(address(oVault_arb), TOKENS_TO_SEND);
@@ -50,6 +76,22 @@ contract OVaultComposerProxySendTest is OVaultComposerBaseTest {
         assertEq(assetOFT_arb.balanceOf(userA), TOKENS_TO_SEND);
     }
 
+    function test_redeemSend_target_hub_can_have_dust() public {
+        uint256 amountWithDust = TOKENS_TO_SEND + 1; // 1 extra token to test dust handling
+
+        SendParam memory sendParam = SendParam(ARB_EID, addressToBytes32(userA), amountWithDust, 0, "", "", "");
+        assetOFT_arb.mint(address(oVault_arb), amountWithDust);
+        oVault_arb.mint(address(userA), amountWithDust);
+
+        vm.startPrank(userA);
+        oVault_arb.approve(address(OVaultComposerArb), amountWithDust);
+        OVaultComposerArb.redeemSend(amountWithDust, sendParam, userA);
+        vm.stopPrank();
+
+        assertEq(oVault_arb.balanceOf(userA), 0);
+        assertEq(assetOFT_arb.balanceOf(userA), amountWithDust);
+    }
+
     function test_depositSend_target_not_hub() public {
         SendParam memory sendParam = SendParam(POL_EID, addressToBytes32(userA), TOKENS_TO_SEND, 0, "", "", "");
         assetOFT_arb.mint(address(userA), TOKENS_TO_SEND);
diff --git a/packages/ovault-evm/test/mocks/MockOVault.sol b/packages/ovault-evm/test/mocks/MockOVault.sol
@@ -28,9 +28,8 @@ contract MockOVaultUpgradeable is OVaultUpgradeable {
         _disableInitializers();
     }
 
-    function __OVault_init(string memory _name, string memory _symbol, address _asset) internal onlyInitializing {
-        __ERC4626_init(IERC20(_asset));
-        __ERC20_init(_name, _symbol);
+    function initialize(string memory _name, string memory _symbol, address _asset) internal onlyInitializing {
+        __OVault_init(_name, _symbol, _asset);
     }
 
     function totalAssets() public view override returns (uint256) {

Original file line number	Diff line number	Diff line change
`@@ -28,9 +28,8 @@ contract MockOVaultUpgradeable is OVaultUpgradeable {`
`28`	`28`	`_disableInitializers();`
`29`	`29`	`}`
`30`	`30`
`31`		`- function __OVault_init(string memory _name, string memory _symbol, address _asset) internal onlyInitializing {`
`32`		`- __ERC4626_init(IERC20(_asset));`
`33`		`- __ERC20_init(_name, _symbol);`
	`31`	`+ function initialize(string memory _name, string memory _symbol, address _asset) internal onlyInitializing {`
	`32`	`+ __OVault_init(_name, _symbol, _asset);`
`34`	`33`	`}`
`35`	`34`
`36`	`35`	`function totalAssets() public view override returns (uint256) {`