ERC4626 Implementation Standard Inconsistency

[Zer0dot]

Found after discussions with @BenSparksCode, @donosonaumczuk and @vicnaum.

We are not certain whether this is a lack of clarity in the standard's description or an inconsistency in the implementation. The standard states that the totalAssets() function "MUST be inclusive of any fees that are charged against assets in the Vault."

However, doing so would include the charged fees in the calculation of the asset <=> share rate in the current implementation, giving an artificially reduced share rate. This either means that, by "inclusive," the EIP means that it should include the removal of fees, or that the implementation should instead remove fees before using the totalAssets() function in conversion calculations.

Relevant code:

    function totalAssets() public view virtual returns (uint256);

    function convertToShares(uint256 assets) public view virtual returns (uint256) {
        uint256 supply = totalSupply; // Saves an extra SLOAD if totalSupply is non-zero.

        return supply == 0 ? assets : assets.mulDivDown(supply, totalAssets());
    }

    function convertToAssets(uint256 shares) public view virtual returns (uint256) {
        uint256 supply = totalSupply; // Saves an extra SLOAD if totalSupply is non-zero.

        return supply == 0 ? shares : shares.mulDivDown(totalAssets(), supply);
    }

    function previewDeposit(uint256 assets) public view virtual returns (uint256) {
        return convertToShares(assets);
    }

    function previewMint(uint256 shares) public view virtual returns (uint256) {
        uint256 supply = totalSupply; // Saves an extra SLOAD if totalSupply is non-zero.

        return supply == 0 ? shares : shares.mulDivUp(totalAssets(), supply);
    }

    function previewWithdraw(uint256 assets) public view virtual returns (uint256) {
        uint256 supply = totalSupply; // Saves an extra SLOAD if totalSupply is non-zero.

        return supply == 0 ? assets : assets.mulDivUp(supply, totalAssets());
    }

    function previewRedeem(uint256 shares) public view virtual returns (uint256) {
        return convertToAssets(shares);
    }

[transmissions11]

ugh yes this seems valid, idk why we made totalAssets account for fees

[androolloyd]

Depending on where the vault fee rake is being captured, asset side or vault share supply side, you may have to adjust your totalSupply to be a function that accounts for this, especially if lazy minting the fee rake later.

going through the spec was def a headache