User can lose 10 ethers to Vault

[code423n4]

Lines of code

https://github.com/AstariaXYZ/astaria-gpl/blob/f430f5b8fe868d4883a8249555df76ee7752587a/src/ERC4626-Cloned.sol#L38-L52

Vulnerability details

Impact

If a user or a contract that has a large allowance (10 ethers or max) on an ERC4626Cloned based Vault that has not yet received any deposits, calls mint with 0 share argument, will have a 10 ethers of the asset transferred to the Vault with no way to reclaim it because he/it will receive no shares.

NOTE: Raising this issue as high risk conforming to judging criteria '3 — High: Assets can be stolen/*lost*/compromised directly'. No complains if judge feels that the necessary pre-conditions makes it a '2 — Med'.

Proof of Concept

    function testAssetsLostToVault() public {
        TestNFT nft = new TestNFT(1);
        address tokenContract = address(nft);
        uint256 tokenId = uint256(0);

        uint256 initialBalance = WETH9.balanceOf(address(this));

        // create a PublicVault with a 14-day epoch
        address publicVault =
            _createPublicVault({strategist: strategistOne, delegate: strategistTwo, epochLength: 14 days});

        PublicVault pv = PublicVault(publicVault);
        address joe = address(0x70a57ed);
        vm.deal(joe, 10 ether);
        vm.startPrank(joe);
        WETH9.deposit{value: 10 ether}();
        WETH9.approve(publicVault, 10 ether);
        uint256 assets = pv.mint(0, joe);

        emit log_string(string.concat("Joe's balance     : ", Strings.toString(WETH9.balanceOf(address(joe)))));
        emit log_string(string.concat("Joe's shares      : ", Strings.toString(pv.balanceOf(address(joe)))));
        emit log_string(string.concat("pv.totalAssets()  : ", Strings.toString(pv.totalAssets())));
        emit log_string(string.concat("pv.totalSupply()  : ", Strings.toString(pv.totalSupply())));
        emit log_string(string.concat("pv.previewRedeem(): ", Strings.toString(pv.previewRedeem(pv.balanceOf(address(joe))))));

        vm.stopPrank();
    }

Test results:

Running 1 test for src/test/AstariaTest.t.sol:AstariaTest
[PASS] testAssetsLostToVault() (gas: 1307633)
Logs:
  Joe's balance     : 0
  Joe's shares      : 0
  pv.totalAssets()  : 10000000000000000000
  pv.totalSupply()  : 0
  pv.previewRedeem(): 0

Test result: ok. 1 passed; 0 failed; finished in 29.48ms

Tools Used

n/a

Recommended Mitigation Steps

Since the previewMint does not use the original supply == 0 ? shares : ..., at least add a require precondition to the mint function to prevent minting of 0 shares.

[c4-judge]

Picodes marked the issue as duplicate of #588

[Picodes]

Giving duplicate with partial credit as this report misses the main impact of the finding which is the possibility for the first user to trick the next ones

[c4-judge]

Picodes marked the issue as partial-50

[eierina]

Hi @Picodes

I trust this to be an oversight, please note that this is not the same issue you are mentioning (and indeed I have another issue open for the same you're mentioning) and it would still be present after the fix to https://github.com/code-423n4/2023-01-astaria-findings/issues/588.

This one is about the risk of calling mint passing 0 for shares argument. In this case, the message sender will lose 10 ethers and receive nothing in exchange therefore having no way to redeem or withdraw.

The original ERC4626 code emits "x" shares if supply is 0, where "x" is the same argument passed to the mint function, so in the original code there is no risk of passing 0 because it has no effects, but in this case it does have an effect and therefore the mint should be protected, which is not. Please check carefully the included test and the test outputs.

[Picodes]

Hi @eierina, indeed I missed that you had another issue opened. In this case, I'll treat both reports separately.

This report successfully notices that there is an issue with previewMint but the scenario described here is strictly speaking of Low severity at best. Indeed, calling the function with 0 as an argument would be an error on the user side as it doesn't make any sense. Therefore, protecting users from this falls within the Safety checks category, which is QA.

[Picodes]

Also, please note that you shouldn't comment on your own findings until post-judging QAs and check the rules here: https://code4rena.notion.site/Backstage-Warden-Guidelines-06d1073540994cc08937f721c2951b0f

[c4-judge]

Picodes marked the issue as satisfactory