code423n4 commented 2 years ago

Lines of code

https://github.com/code-423n4/2022-11-redactedcartel/blob/9e9bb60f117334da7c5d851646a168ca271575fc/src/vaults/AutoPxGmx.sol#L199

Vulnerability details

Proof of Concept

In both contracts, the previewWithdraw function has the same body

// Calculate shares based on the specified assets' proportion of the pool
        uint256 shares = convertToShares(assets);

        // Save 1 SLOAD
        uint256 _totalSupply = totalSupply;

        // Factor in additional shares to fulfill withdrawal if user is not the last to withdraw
        return
            (_totalSupply == 0 || _totalSupply - shares == 0)
                ? shares
                : (shares * FEE_DENOMINATOR) /
                    (FEE_DENOMINATOR - withdrawalPenalty);

The problem is that the ERC4626 standard specifically requires previewWithdraw to round up, while this code does not do so - actually convertToShares rounds down.

previewWithdraw returns the amount of shares that would be burned to withdraw specific amount of asset. Thus, the amount of shares must to be rounded up, so that the vault won't be shortchanged.

Impact

Since contracts are not compliant with a standard this can result in some integration issues and in this particular case it can also result in a wrong amount returned from previewWithdraw which won’t actually work with withdraw, so it can cause lots of reverted transactions. Medium severity is appropriate here.