novaman33 - _splitWithdrawRequest will make invalid withdraw requests in an edge case

[sherlock-admin4]

novaman33

High

_splitWithdrawRequest will make invalid withdraw requests in an edge case

Summary

When an account is deleveraged, _splitWithdrawRequest is called so that pending withdraw requests of the account that is being liquidated are split between them and the liquidator. However when the account is being fully liquidated, the old withdraw request is deleted which creates an invalid Id for the liquidator's withdrawRequest.

Vulnerability Detail

In _splitWithdrawRequest the request of the from address is being read by storage:

 WithdrawRequest storage w = VaultStorage.getAccountWithdrawRequest()[_from];

Then the following check is made to delete the withdraw request of the from account if all the vault tokens are being taken from him:

 if (w.vaultShares == vaultShares) {
            // If the resulting vault shares is zero, then delete the request. The _from account's
            // withdraw request is fully transferred to _to
            delete VaultStorage.getAccountWithdrawRequest()[_from];
        } 

Here the delete keyword is used to reset the withdraw request of the from account. However the w variable is still a pointer to this place in storage meaning that resetting VaultStorage.getAccountWithdrawRequest()[_from] will also be resetting w. As a result w.requestId=0 and

toWithdraw.requestId = w.requestId;

Here the new requestId is equal to 0 which is the default value meaning that this withdraw request will not be recognized by _finalizeWithdrawsManual and the other finalize withdraw functions and all these vault shares will be lost. Also if initiateWithdraw is called the old vaultShares will be wiped out for the new shares to be withdrawn.

Impact

Loss of vaut tokens for the liquidator.

Code Snippet

https://github.com/sherlock-audit/2024-06-leveraged-vaults/blob/14d3eaf0445c251c52c86ce88a84a3f5b9dfad94/leveraged-vaults-private/contracts/vaults/common/WithdrawRequestBase.sol#L221

Tool used

Manual Review

Recommendation

Store the requestId of the from address in another memory variable.

[sherlock-admin2]

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

0xmystery commented:

w has been assigned and will not be reset

[novaman33]

Escalate,
This is a valid issue. w is a pointer and is being reset when mapping is deleted.

[sherlock-admin3]

Escalate,
This is a valid issue. w is a pointer and is being reset when mapping is deleted.

You've created a valid escalation!

To remove the escalation from consideration: Delete your comment.

You may delete or edit your escalation comment anytime before the 48-hour escalation window closes. After that, the escalation becomes final.

[amankakar]

This Issue is dup of #89

You can observe that there is no return or halt in the flow when a request is deleted. The same requestID, which has been deleted, retains its storage scope. Consequently, the requestID assigned to the new withdrawal request for the liquidator will always be zero in this scenario. This is a significant issue that could result in asset loss and needs to be reconsidered.
For proof, I have also added simple code demonstrating that the requestID will be 0 after deletion.

[mystery0x]

This Issue is dup of #89

You can observe that there is no return or halt in the flow when a request is deleted. The same requestID, which has been deleted, retains its storage scope. Consequently, the requestID assigned to the new withdrawal request for the liquidator will always be zero in this scenario. This is a significant issue that could result in asset loss and needs to be reconsidered. For proof, I have also added simple code demonstrating that the requestID will be 0 after deletion.

Please provide a coded POC alleging your claim. I will also request the sponsors looking into your report.

[novaman33]

@mystery0x

// SPDX-License-Identifier: UNLICENSED

pragma solidity 0.8.22;

import "forge-std/Test.sol";

struct WithdrawRequest {
    uint256 requestId;
    uint256 vaultShares;
    bool hasSplit;
}
contract TestRequestID is Test {
    address alice = makeAddr("aLice");
    address bob = makeAddr("bob");

    struct SplitWithdrawRequest {
        uint256 totalVaultShares; // uint64
        uint256 totalWithdraw; // uint184?
        bool finalized;
    }

    function setUp() external {}

    uint256 public constant DISTRIBUTION_DIVISOR = 100 ether;

    function testRequestId() external {
        _createRequest();
        _splitWithdrawRequest(address(alice), address(bob), 100);
        WithdrawRequest storage w = VaultStorage.getAccountWithdrawRequest()[
            bob
        ];
        assert(w.requestId == 0);
    }

    function _splitWithdrawRequest(
        address _from,
        address _to,
        uint256 vaultShares
    ) internal {
        WithdrawRequest storage w = VaultStorage.getAccountWithdrawRequest()[
            _from
        ];
        if (w.requestId == 0) return;
        // @audit : Ingore this code as it does not have any impact in our case 
        // Create a new split withdraw request
        // if (!w.hasSplit) {
        //     SplitWithdrawRequest memory s = VaultStorage
        //         .getSplitWithdrawRequest()[w.requestId];
        //     // Safety check to ensure that the split withdraw request is not active, split withdraw
        //     // requests are never deleted. This presumes that all withdraw request ids are unique.
        //     require(s.finalized == false && s.totalVaultShares == 0);
        //     VaultStorage
        //     .getSplitWithdrawRequest()[w.requestId].totalVaultShares = w
        //         .vaultShares;
        // }

        console.log("Request ID is Before Delete :", w.requestId);

        if (w.vaultShares == vaultShares) {
            // If the resulting vault shares is zero, then delete the request. The _from account's
            // withdraw request is fully transferred to _to
            delete VaultStorage.getAccountWithdrawRequest()[_from];
        } else {
            // Otherwise deduct the vault shares
            w.vaultShares = w.vaultShares - vaultShares;
            w.hasSplit = true;
        }

        // Ensure that no withdraw request gets overridden, the _to account always receives their withdraw
        // request in the account withdraw slot.
        WithdrawRequest storage toWithdraw = VaultStorage
            .getAccountWithdrawRequest()[_to];
        require(
            toWithdraw.requestId == 0 || toWithdraw.requestId == w.requestId,
            "Existing Request"
        );

        console.log("Request ID is After Delete :", w.requestId);
        // Either the request gets set or it gets incremented here.
        toWithdraw.requestId = w.requestId; // @audit : the request id will be zero here
        toWithdraw.vaultShares = toWithdraw.vaultShares + vaultShares;
        toWithdraw.hasSplit = true;
    }

    function _createRequest() internal {
        WithdrawRequest storage accountWithdraw = VaultStorage
            .getAccountWithdrawRequest()[alice];

        accountWithdraw.requestId = uint256(uint160(address(alice)));
        accountWithdraw.vaultShares = 100;
        accountWithdraw.hasSplit = false;
    }
}

library VaultStorage {
    /// @notice Emitted when vault settings are updated
    // event StrategyVaultSettingsUpdated(StrategyVaultSettings settings);
    // Wrap timestamp in a struct so that it can be passed around as a storage pointer
    struct LastClaimTimestamp {
        uint256 value;
    }

    /// @notice account initiated WithdrawRequest
    uint256 private constant ACCOUNT_WITHDRAW_SLOT = 1000008;

    function getAccountWithdrawRequest()
        internal
        pure
        returns (mapping(address => WithdrawRequest) storage store)
    {
        assembly {
            store.slot := ACCOUNT_WITHDRAW_SLOT
        }
    }
}

  Request ID is Before Delete : 964133639515395071211053928642046522704087555788
  Request ID is After Delete : 0

[WangSecurity]

I agree with the escalation, it is a nice find. Planning to accept it and validate it with High severity since it will happen with every full liquidation. The duplicate is #89, @mystery0x are there other duplicates as well?

[wtp0x]

#41

[WangSecurity]

Result:
High
Has duplicates

[sherlock-admin2]

Escalations have been resolved successfully!

Escalation status:

• novaman33: accepted

[mystery0x]

I agree with the escalation, it is a nice find. Planning to accept it and validate it with High severity since it will happen with every full liquidation. The duplicate is #89, @mystery0x are there other duplicates as well?

#6, #41, and #89 are the only three reports submitting this finding.

[sherlock-admin2]

The protocol team fixed this issue in the following PRs/commits:
notional-finance/leveraged-vaults#106

[sherlock-admin2]

The Lead Senior Watson signed off on the fix.