sherlock-audit / 2024-05-napier-update-judging

8 stars 7 forks source link

merlin - PufETHAdapter has not implemented withdrawal functions #51

Closed sherlock-admin3 closed 3 months ago

sherlock-admin3 commented 3 months ago

merlin

medium

PufETHAdapter has not implemented withdrawal functions

Summary

There are many NatSpec comments stating that Puffer does not have a withdrawal function yet. However, this is not true.

Vulnerability Detail

/// @dev Puffer doesn't have withdraw function yet.
    function requestWithdrawal() external pure override {
        revert NotImplemented();
    }

In the older version of PufferVault.sol, the withdraw and redeem functions were indeed not implemented:

/**
     * @notice Not allowed
     */
    function redeem(uint256, address, address) public virtual override returns (uint256) {
        revert WithdrawalsAreDisabled();
    }

    /**
     * @notice Not allowed
     */
    function withdraw(uint256, address, address) public virtual override returns (uint256) {
        revert WithdrawalsAreDisabled();
    }

However, in the new version, PufferVaultV2.sol, these functions are now present.

function withdraw(uint256 assets, address receiver, address owner)
        public
        virtual
        override
        revertIfDeposited
        restricted
        returns (uint256)
    {
        uint256 maxAssets = maxWithdraw(owner);
        if (assets > maxAssets) {
            revert ERC4626ExceededMaxWithdraw(owner, assets, maxAssets);
        }

        _updateDailyWithdrawals(assets);

        _wrapETH(assets);

        uint256 shares = previewWithdraw(assets);
        _withdraw({ caller: _msgSender(), receiver: receiver, owner: owner, assets: assets, shares: shares });

        return shares;
    }

    function redeem(uint256 shares, address receiver, address owner)
        public
        virtual
        override
        revertIfDeposited
        restricted
        returns (uint256)
    {
        uint256 maxShares = maxRedeem(owner);
        if (shares > maxShares) {
            revert ERC4626ExceededMaxRedeem(owner, shares, maxShares);
        }

        uint256 assets = previewRedeem(shares);

        _updateDailyWithdrawals(assets);

        _wrapETH(assets);

        _withdraw({ caller: _msgSender(), receiver: receiver, owner: owner, assets: assets, shares: shares });

        return assets;
    }

The only action required from the Puffer protocol is to allow everyone to call these functions, as there is currently a restricted modifier that functions similarly to the whenNotPaused modifier from Pausable.sol. Essentially, the Puffer protocol will allow withdrawals soon by changing access to the functions

* @dev Restricted in this context is like `whenNotPaused` modifier from Pausable.sol

Impact

The Napier protocol stated that Puffer doesn't have a withdrawal function yet, which is why they didn't implement withdrawal functions. However, this is not true.

Code Snippet

src/adapters/puffer/PufETHAdapter.sol#L89-L102

Tool used

Manual Review

Recommendation

The PufferVaultV2.sol address hasn't changed, only the implementation has. Consider implementing withdrawal functions.

sherlock-admin3 commented 3 months ago

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

z3s commented:

Low/Info; The functionality is not currently available, so it's future issue concern and is therefore invalid.