code-423n4 / 2023-05-maia-findings

23 stars 12 forks source link

the mint function in erc4626 will mint incorrect amount #913

Closed code423n4 closed 1 year ago

code423n4 commented 1 year ago

Lines of code

https://github.com/code-423n4/2023-05-maia/blob/54a45beb1428d85999da3f721f923cbf36ee3d35/src/erc-4626/ERC4626.sol#L53-L54

Vulnerability details

Impact

if you look at the ERC4626 contract the function mint minting the wrong amount at line 53 it should be minting the assets amount not the amount of the share and because the shares and assets are not 1:1 it will lead to unwanted results and different mint amounts.

Proof of Concept

the ERC4626 codes are written from solmate contract but in the mint function, there is a difference that will cause a difference in the calculation of mints which is very mandatory to consider.

this is the mint function of ERC4626 of this protocol

  /// @inheritdoc IERC4626
    function mint(uint256 shares, address receiver) public virtual returns (uint256 assets) {
        assets = previewMint(shares); // No need to check for rounding error, previewMint rounds up.

        // Need to transfer before minting or ERC777s could reenter.
        address(asset).safeTransferFrom(msg.sender, address(this), assets);

        _mint(receiver, shares);

        emit Deposit(msg.sender, receiver, assets, shares);

        afterDeposit(assets, shares);
    }

and now look at the original contract that is developed by Solmate team

   function mint(uint256 shares, address to) public virtual returns (uint256 amount) {
        amount = previewMint(shares); // No need to check for rounding error, previewMint rounds up.

        // Need to transfer before minting or ERC777s could reenter.
        asset.safeTransferFrom(msg.sender, address(this), amount);

        _mint(to, amount);

        emit Deposit(msg.sender, to, amount, shares);

        afterDeposit(amount, shares);
    }

the difference is in _mint(to, amount);(solmate) part which is written like that in protocol _mint(receiver, shares);

the protocol uses solmate structure as erc4626 in every single function of erc4626 and its the same but there is a difference in the mint part that will cause a difference in the results of mints and here in the protocol since the assets (amount ) and the shares are not 1:1 it will cause dangerous problems for the codebase.

Tools Used

vs code/ manually audited

Recommended Mitigation Steps

implement the function in that way

   function mint(uint256 shares, address to) public virtual returns (uint256 amount) {
        amount = previewMint(shares); // No need to check for rounding error, previewMint rounds up.

        // Need to transfer before minting or ERC777s could reenter.
        asset.safeTransferFrom(msg.sender, address(this), amount);

        _mint(to, amount);

        emit Deposit(msg.sender, to, amount, shares);

        afterDeposit(amount, shares);
    }

use + _mint(to, amount); https://github.com/transmissions11/solmate/blob/8c0e278900fe552fa0739975bde21c6a07d84ccf/src/mixins/ERC4626.sol#L67

instead of

_mint(receiver, shares);

Assessed type

ERC4626

c4-judge commented 1 year ago

trust1995 marked the issue as unsatisfactory: Insufficient proof