functions withdraw() and deposit() in Vault contract use shares variable instead of assets to transfer user assets

[code423n4]

Lines of code

https://github.com/code-423n4/2022-09-y2k-finance/blob/2175c044af98509261e4147edeb48e1036773771/src/Vault.sol#L145-L174
https://github.com/code-423n4/2022-09-y2k-finance/blob/2175c044af98509261e4147edeb48e1036773771/src/Vault.sol#L195-L234

Vulnerability details

Impact

in functions deposit() and withdraw() of Vault contract, user specify how much assets he want to deposit to the contract or withdraw from contract and contract calculates the corresponding share amount for that amount of assets and mint or burn those shares for user. the code of deposit() and withdraw has been written in general way to handle general cases but code uses variable shares instead of assets to transfer user assets. This is wrong for general cases and only works when assets equal to shares and ratio is 1 to 1 between token and shares. as everything in code is written in general manner, in this cases code shouldn't written for special cases when the ratio is 1 to 1 too.

Proof of Concept

This is deposit() and withdraw() code in Vault contract:

    /**
        @notice Deposit function from ERC4626, with payment of a fee to a treasury implemented;
        @param  id  uint256 in UNIX timestamp, representing the end date of the epoch. Example: Epoch ends in 30th June 2022 at 00h 00min 00sec: 1654038000;
        @param  assets  uint256 representing how many assets the user wants to deposit, a fee will be taken from this value;
        @param receiver  address of the receiver of the shares provided by this function, that represent the ownership of the deposited asset;
        @return shares how many assets the owner is entitled to, removing the fee from it's shares;
     */
    function deposit(
        uint256 id,
        uint256 assets,
        address receiver
    )
        public
        override
        marketExists(id)
        epochHasNotStarted(id)
        nonReentrant
        returns (uint256 shares)
    {
        // Check for rounding error since we round down in previewDeposit.
        require((shares = previewDeposit(id, assets)) != 0, "ZeroValue");

        asset.transferFrom(msg.sender, address(this), shares);

        _mint(receiver, id, shares, EMPTY);

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

        return shares;
    }

    /**
    @notice Withdraw entitled deposited assets, checking if a depeg event //TODO add GOV token rewards
    @param  id uint256 in UNIX timestamp, representing the end date of the epoch. Example: Epoch ends in 30th June 2022 at 00h 00min 00sec: 1654038000;
    @param assets   uint256 of how many assets you want to withdraw, this value will be used to calculate how many assets you are entitle to according to the events;
    @param receiver  Address of the receiver of the assets provided by this function, that represent the ownership of the transfered asset;
    @param owner    Address of the owner of these said assets;
    @return shares How many shares the owner is entitled to, according to the conditions;
     */
    function withdraw(
        uint256 id,
        uint256 assets,
        address receiver,
        address owner
    )
        external
        override
        epochHasEnded(id)
        marketExists(id)
        returns (uint256 shares)
    {
        if(
            msg.sender != owner &&
            isApprovedForAll(owner, receiver) == false)
            revert OwnerDidNotAuthorize(msg.sender, owner);

        shares = previewWithdraw(id, assets); // No need to check for rounding error, previewWithdraw rounds up.

        uint256 entitledShares = beforeWithdraw(id, shares);
        _burn(owner, id, shares);

        //Taking fee from the amount
        uint256 feeValue = calculateWithdrawalFeeValue(entitledShares, id);
        entitledShares = entitledShares - feeValue;
        asset.transfer(treasury, feeValue);

        emit Withdraw(msg.sender, receiver, owner, id, assets, entitledShares);
        asset.transfer(receiver, entitledShares);

        return entitledShares;
    }

As you can see in this line asset.transferFrom(msg.sender, address(this), shares) in deposit() function code uses shares to transfer user assets and deposit them to contract and shares is calculated based on assets in the previous line (shares = previewDeposit(id, assets)) and user specified amount for depositing is assets. so as whole code is written for general cases here code assumes that shares equal to assets and the logics is wrong for general case Vault.
As you can see in this line uint256 entitledShares = beforeWithdraw(id, shares) in withdraw() function, code uses shares to calculate amount of assets user is going to get, beforeWithdraw() is accepting amount of assets user wants to withdraw but here code send shares to that function. this logic is incorrect for general case Vault and only works when shares = assets and the ratio is 1 to 1. This is beforeWithdraw() code which says it requires the second parameter to be amount of assets user wants to withdraw:

/**
    @notice Calculations of how much the user will receive;
    @param  id uint256 in UNIX timestamp, representing the end date of the epoch. Example: Epoch ends in 30th June 2022 at 00h 00min 00sec: 1654038000
    @param amount uint256 of the amount the user wants to withdraw
    @return entitledAmount How much amount the user will receive, according to the conditions
    */
    function beforeWithdraw(uint256 id, uint256 amount)
        public
        view
        returns (uint256 entitledAmount)
    {
        // in case the risk wins aka no depeg event
        // risk users can withdraw the hedge (that is paid by the hedge buyers) and risk; withdraw = (risk + hedge)
        // hedge pay for each hedge seller = ( risk / tvl before the hedge payouts ) * tvl in hedge pool
        // in case there is a depeg event, the risk users can only withdraw the hedge
        if (

So as showed the logics of are wrong for general cases and all of the code is written in a manner that is could work for general cases but here the logic of using shares instead of assets is not for general cases. if code is based on the fact that asset to share ratio is 1 to 1 then there was no need to call previewWithdraw() or previewDeposit() and most of conversation logics between shares and assets.

Tools Used

VIM

Recommended Mitigation Steps

use assets instead of shares

[HickupHH3]

dup #471

part of #253