Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Missing maxwithdraw check in withdraw function of ERC-4626 #362

Open
pcaversaccio opened this issue Feb 24, 2023 · 1 comment
Open

Missing maxwithdraw check in withdraw function of ERC-4626 #362

pcaversaccio opened this issue Feb 24, 2023 · 1 comment

Comments

@pcaversaccio
Copy link

pcaversaccio commented Feb 24, 2023

In the EIP-4626 specification it reads:

maxwithdraw
Maximum amount of the underlying asset that can be withdrawn from the owner balance in the Vault, through a withdraw call.

However, the current implementation misses this check:

    function withdraw(
        uint256 assets,
        address receiver,
        address owner
    ) public virtual returns (uint256 shares) {
        shares = previewWithdraw(assets); // No need to check for rounding error, previewWithdraw rounds up.

        if (msg.sender != owner) {
            uint256 allowed = allowance[owner][msg.sender]; // Saves gas for limited approvals.

            if (allowed != type(uint256).max) allowance[owner][msg.sender] = allowed - shares;
        }

        beforeWithdraw(assets, shares);

        _burn(owner, shares);

        emit Withdraw(msg.sender, receiver, owner, assets, shares);

        asset.safeTransfer(receiver, assets);
    }

It should be considered adding something like that (I don't assume beforeWithdraw hook should be used for that):

require(assets <= maxWithdraw(owner), "ERC4626: withdraw more than max");

Furthermore, similar checks are missing in deposit, mint, and redeem.

@advock
Copy link

advock commented Apr 20, 2023

I have made the changes. Creating a pull request

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants