search

sherlock-audit / 2024-01-napier-judging

9 stars 6 forks source link

aman - `SFrxETHAdapter:requestWithdrawal` and `SFrxETHAdapter:requestWithdrawalAll` return wrong requestId, would result in DOS #24

Closed sherlock-admin2 closed 7 months ago

sherlock-admin2 commented 7 months ago

aman

high

SFrxETHAdapter:requestWithdrawal and SFrxETHAdapter:requestWithdrawalAll return wrong requestId, would result in DOS

Summary

The Rebalancer wants to withdraw the staked ETH from the Frax protocol. They call BaseLSTAdapter:requestWithdrawal for a bufferAmount or to withdraw all staking. For the latter, they call SFrxETHAdapter::requestWithdrawalAll. Due to using the cached requestId, it is very likely that the requestId against which the NFT is minted and stored on the Frax protocol will be different from the one stored on the Napier side. The following POC would help to understand the use case.

Vulnerability Detail

The Rebalancer wants to withdraw the staked ETH from the Frax protocol. They call BaseLSTAdapter:requestWithdrawal for a bufferAmount or to withdraw all staking. For the latter, they call SFrxETHAdapter::requestWithdrawalAll. Due to using the cached requestId, it is very likely that the requestId against which the NFT is minted and stored on the Frax protocol will be different from the one stored on the Napier side. The following POC would help to understand the use case.

1). Let the Rebalancer call the BaseLSTAdapter:requestWithdrawal function to withdraw staked tokens from Frax. The contract first retrieves the nextNft from FraxEtherRedemptionQueue by calling FraxEtherRedemptionQueue::redemptionQueueState().nextNftId. Let's suppose the requestId returned is 1.

2).Meanwhile, Bob calls FraxEtherRedemptionQueue:enterRedemptionQueue. The FraxEtherRedemptionQueue completes Bob's transaction and returns requestId 1 to Bob.

3). The Rebalancer transaction completes the STAKED_FRXETH.withdraw(withdrawAmount, address(this), address(this)); and submits the requestWithdrawal REDEMPTION_QUEUE.enterRedemptionQueue({amountToRedeem: frxEthWithdrawn.toUint120(), recipient: address(this)}); Since requestId 1 was already used and now the nextNftId is 2, the FraxEtherRedemptionQueue:enterRedemptionQueue records the Rebalancer request against requestId 2.

4). The BaseLSTAdapter stores requestId 1, but the requestId of BaseLSTAdapter is 2.

https://github.com/sherlock-audit/2024-01-napier/blob/6313f34110b0d12677b389f0ecb3197038211e12/napier-v1/src/adapters/frax/SFrxETHAdapter.sol#L99

https://github.com/sherlock-audit/2024-01-napier/blob/6313f34110b0d12677b389f0ecb3197038211e12/napier-v1/src/adapters/frax/SFrxETHAdapter.sol#L108

https://github.com/sherlock-audit/2024-01-napier/blob/6313f34110b0d12677b389f0ecb3197038211e12/napier-v1/src/adapters/frax/SFrxETHAdapter.sol#L83

https://github.com/sherlock-audit/2024-01-napier/blob/6313f34110b0d12677b389f0ecb3197038211e12/napier-v1/src/adapters/frax/SFrxETHAdapter.sol#L89

This will cause all the calls claimWithdrawal, _requestWithdrawal, and requestWithdrawalAll functions of the SFrxETHAdapter contract to fail.

Impact

Due to wrong requestId All the Assets stacked will stuck at FraxEtherRedemptionQueue ,, no new RequestWithdrawal will be requested, and there is no way to recover contract from this state.

Code Snippet


    function requestWithdrawalAll() external override nonReentrant onlyRebalancer {
        if (requestId != 0) revert WithdrawalPending();
 @>       uint256 _requestId = REDEMPTION_QUEUE.redemptionQueueState().nextNftId;
        /// INTERACT ///
        // Redeem all sfrxETH for frxETH
        uint256 balance = STAKED_FRXETH.balanceOf(address(this));
        uint256 withdrawAmount = STAKED_FRXETH.redeem(balance, address(this), address(this));

        REDEMPTION_QUEUE.enterRedemptionQueue({amountToRedeem: withdrawAmount.toUint120(), recipient: address(this)});

        /// WRITE ///
 @>       withdrawalQueueEth = REDEMPTION_QUEUE.nftInformation(_requestId).amount; // cast uint120 to uint128
        requestId = _requestId;
    }

function _requestWithdrawal(uint256 withdrawAmount) internal override returns (uint256, uint256) {
@>        uint256 _requestId = REDEMPTION_QUEUE.redemptionQueueState().nextNftId; // Dev: Ensure id is not 0
        // @audit : the requestID != 0 check missing
        /// INTERACT ///
        uint256 frxEthBalanceBefore = FRXETH.balanceOf(address(this)); // 0 is expected if no one has donated frxETH to this contract
        STAKED_FRXETH.withdraw(withdrawAmount, address(this), address(this));
        uint256 frxEthWithdrawn = FRXETH.balanceOf(address(this)) - frxEthBalanceBefore;
        // Transfer frxETH and mint redemption ticket.
        // note: `amountToRedeem` is an amount in frxETH, not ETH.
        // However, frxETH would be soft-pegged to ETH, so we treat them as 1:1 for simplicity here.
        // Also, actual ETH amount to withdraw would be slightly less than `withdrawAmount` due to the redemption fee.
        // @audit : instead of caching the _request Id befor requesting from queue , instead store the return values from  enterRedemptionQueue.
 @>       REDEMPTION_QUEUE.enterRedemptionQueue({amountToRedeem: frxEthWithdrawn.toUint120(), recipient: address(this)});
        /// WRITE ///
        // Note: The redemption queue contract returns the exact amount of ETH to withdraw.
        // @audit : chance to report wrong amount.
 @>       uint256 queueEth = REDEMPTION_QUEUE.nftInformation(_requestId).amount; // it will return the wrong amount because the requestID is wrong.
        return (queueEth, _requestId);
    }

Tool used

Manual Review

Recommendation

REDEMPTION_QUEUE.enterRedemptionQueue return the nftId (aka requestId). instead of caching the requestId before ahead use the return requestId.

diff --git a/napier-v1/src/adapters/frax/SFrxETHAdapter.sol b/napier-v1/src/adapters/frax/SFrxETHAdapter.sol
index 9e1be29..6fc21b1 100644
--- a/napier-v1/src/adapters/frax/SFrxETHAdapter.sol
+++ b/napier-v1/src/adapters/frax/SFrxETHAdapter.sol
@@ -80,13 +80,12 @@ contract SFrxETHAdapter is BaseLSTAdapter, IERC721Receiver {

     function requestWithdrawalAll() external override nonReentrant onlyRebalancer {
         if (requestId != 0) revert WithdrawalPending();
-        uint256 _requestId = REDEMPTION_QUEUE.redemptionQueueState().nextNftId;
         /// INTERACT ///
         // Redeem all sfrxETH for frxETH
         uint256 balance = STAKED_FRXETH.balanceOf(address(this));
         uint256 withdrawAmount = STAKED_FRXETH.redeem(balance, address(this), address(this));

-        REDEMPTION_QUEUE.enterRedemptionQueue({amountToRedeem: withdrawAmount.toUint120(), recipient: address(this)});
+        uint256 _requestId= REDEMPTION_QUEUE.enterRedemptionQueue({amountToRedeem: withdrawAmount.toUint120(), recipient: address(this)});

         /// WRITE ///
         withdrawalQueueEth = REDEMPTION_QUEUE.nftInformation(_requestId).amount; // cast uint120 to uint128
@@ -96,7 +95,7 @@ contract SFrxETHAdapter is BaseLSTAdapter, IERC721Receiver {
     /// @notice Request about `withdrawAmount` of ETH to be unstaked from sfrxETH.
     /// @param withdrawAmount Amount of ETH to withdraw
     function _requestWithdrawal(uint256 withdrawAmount) internal override returns (uint256, uint256) {
-        uint256 _requestId = REDEMPTION_QUEUE.redemptionQueueState().nextNftId; // Dev: Ensure id is not 0
+        // @audit : the requestID != 0 check missing
         /// INTERACT ///
         uint256 frxEthBalanceBefore = FRXETH.balanceOf(address(this)); // 0 is expected if no one has donated frxETH to this contract
         STAKED_FRXETH.withdraw(withdrawAmount, address(this), address(this));
@@ -105,9 +104,10 @@ contract SFrxETHAdapter is BaseLSTAdapter, IERC721Receiver {
         // note: `amountToRedeem` is an amount in frxETH, not ETH.
         // However, frxETH would be soft-pegged to ETH, so we treat them as 1:1 for simplicity here.
         // Also, actual ETH amount to withdraw would be slightly less than `withdrawAmount` due to the redemption fee.
-        REDEMPTION_QUEUE.enterRedemptionQueue({amountToRedeem: frxEthWithdrawn.toUint120(), recipient: address(this)});
+        uint256 _requestId =  REDEMPTION_QUEUE.enterRedemptionQueue({amountToRedeem: frxEthWithdrawn.toUint120(), recipient: address(this)});
         /// WRITE ///
         // Note: The redemption queue contract returns the exact amount of ETH to withdraw.
+        // @audit : chance to report wrong amount.
         uint256 queueEth = REDEMPTION_QUEUE.nftInformation(_requestId).amount; // cast uint120 to uint128
         return (queueEth, _requestId);
     }
diff --git a/napier-v1/src/adapters/frax/interfaces/IFraxEtherRedemptionQueue.sol b/napier-v1/src/adapters/frax/interfaces/IFraxEtherRedemptionQueue.sol
index 47bc518..68da346 100644
--- a/napier-v1/src/adapters/frax/interfaces/IFraxEtherRedemptionQueue.sol
+++ b/napier-v1/src/adapters/frax/interfaces/IFraxEtherRedemptionQueue.sol
@@ -82,7 +82,8 @@ interface IFraxEtherRedemptionQueue {
     /// @param recipient Recipient of the NFT. Must be ERC721 compatible if a contract
     /// @param amountToRedeem Amount to redeem
     /// @dev Must call approve/permit on frxEth contract prior to this call
-    function enterRedemptionQueue(address recipient, uint120 amountToRedeem) external;
+    // function enterRedemptionQueue(address recipient, uint120 amountToRedeem) external;
+    function enterRedemptionQueue(address recipient, uint120 amountToRedeem) external returns (uint256 _nftId);
sherlock-admin commented 7 months ago

2 comment(s) were left on this issue during the judging contest.

tsvetanovv commented:

That would be Admin mistake

takarez commented:

valid: This seem valid to me as the watson explained; high(5)

amankakar commented 7 months ago

I think the issue is misunderstood. The NextNftId is a state varaible in FraxEtherRedemptionQueue contract which get incremented after when ever new request is submitted, which can be used by multiple parties. So, there is a high chance of the requestId getting mismatched because Napier uses cached requestId. Additionally, FraxEtherRedemptionQueue returns a requestId whenever a new request is created. you can have a look here:

// Napier calls this function
  function enterRedemptionQueue(address _recipient, uint120 _amountToRedeem) public returns (uint256 _nftId) {
        // Do all of the NFT-generating and accounting logic
        _nftId = _enterRedemptionQueueCore(_recipient, _amountToRedeem);

        // Interactions: Transfer frxEth in from the sender
        IERC20(address(FRX_ETH)).safeTransferFrom({ from: msg.sender, to: address(this), value: _amountToRedeem });
    }
// 
 function _enterRedemptionQueueCore(address _recipient, uint120 _amountToRedeem) internal nonReentrant returns (uint256 _nftId) {
        // Get queue information
        RedemptionQueueState memory _redemptionQueueState = redemptionQueueState;
        RedemptionQueueAccounting memory _redemptionQueueAccounting = redemptionQueueAccounting;

        // Calculations: redemption fee
        uint120 _redemptionFeeAmount = ((uint256(_amountToRedeem) * _redemptionQueueState.redemptionFee) /
            FEE_PRECISION).toUint120();

        // Calculations: amount of ETH owed to the user
        uint120 _amountEtherOwedToUser = _amountToRedeem - _redemptionFeeAmount;

        // Calculations: increment ether liabilities by the amount of ether owed to the user
        _redemptionQueueAccounting.etherLiabilities += uint128(_amountEtherOwedToUser);

        // Calculations: increment unclaimed fees by the redemption fee taken
        _redemptionQueueAccounting.unclaimedFees += _redemptionFeeAmount;

        // Calculations: maturity timestamp
        uint64 _maturityTimestamp = uint64(block.timestamp) + _redemptionQueueState.queueLengthSecs;

        // Effects: Initialize the redemption ticket NFT information
        nftInformation[_redemptionQueueState.nextNftId] = RedemptionQueueItem({
            amount: _amountEtherOwedToUser,
            maturity: _maturityTimestamp,
            hasBeenRedeemed: false,
            earlyExitFee: _redemptionQueueState.earlyExitFee
        });

        // Effects: Mint the redemption ticket NFT. Make sure the recipient supports ERC721.
        _safeMint({ to: _recipient, tokenId: _redemptionQueueState.nextNftId });

        // Emit here, before the state change
        _nftId = _redemptionQueueState.nextNftId;
        emit EnterRedemptionQueue({
            nftId: _nftId,
            sender: msg.sender,
            recipient: _recipient,
            amountFrxEthRedeemed: _amountToRedeem,
            redemptionFeeAmount: _redemptionFeeAmount,
            maturityTimestamp: _maturityTimestamp,
            earlyExitFee: _redemptionQueueState.earlyExitFee
        });

        // Calculations: Increment the autoincrement
        ++_redemptionQueueState.nextNftId;

        // Effects: Write all of the state changes to storage
        redemptionQueueState = _redemptionQueueState;

        // Effects: Write all of the accounting changes to storage
        redemptionQueueAccounting = _redemptionQueueAccounting;
    }

https://etherscan.io/address/0x82bA8da44Cd5261762e629dd5c605b17715727bd#code There are basically two issues in this implementation: 1). The protocol does not integrate with FraxEthAdapter as expected. 2). All the requests highly depend on the requestId. If the requestId stored in this contract is not the one owned by this contract, it will always revert and create a permanent denial-of-service (DOS) issue.