hats-finance / Convergence-Finance---IBO-0x0e410e7af8e70fc5bffcdbfbdf1673ee7b3d0777

IBO, Vesting & Bond mecanism repo prepared for Hat finance audit competition
0 stars 0 forks source link

Max CVG per Bond calculation is incorrect and will cause users to be able to mint much less then expected #15

Open hats-bug-reporter[bot] opened 1 year ago

hats-bug-reporter[bot] commented 1 year ago

Github username: @8ahoz Submission hash (on-chain): 0x04e7fb03d4f60acd19c8e20675e591d941d9d7e2bbb1d85509458ed0ad0ae638 Severity: medium

Description: Description: Ibo.sol contract has a check that assures a user can not mint more then a percentage of the maxCvgToMint.

require(
            cvgToSold <= (_bondParams.percentageMaxCvgToMint * _bondParams.maxCvgToMint) / 10 ** 3,
            "MAX_CVG_PER_BOND"
        );

That percentage is stored in BondParams as percentageMaxCvgToMint. Checking the comment about the parameters shows that the percentage was supposed to be stored as a double digit percentage value:

struct BondParams {
        uint8 composedFunction;
        IERC20Metadata token;
        uint24 gamma;
        uint16 scale;
        uint24 minRoi; // Min bond ROI, divide by 1000 to get the roi in %
        uint24 maxRoi; // Max bond ROI, divide by 1000 to get the roi in %
        uint256 percentageMaxCvgToMint; // Percentage maximum of the maxCvgToMint that an user can mint in one deposit
        uint256 maxCvgToMint; // Limit of Max CVG to mint
    }

Please note the difference between the comments of minRoi, maxRoi and percentageMaxCvgToMint. Which shows percentageMaxCvgToMint value supposed to be a two digit value that supposed to be denominated by 100(10^2)

config.js in the scripts folder proves this inference:

const REAL_IBO_PARAMETERS = {
    FRAX: {
        BOND_PARAMETERS: {
            composedFunction: 0,
            token: TOKENS.FRAX.address,
            gamma: 250_000, // 0.25%
            scale: 5_000, // 0.5%
            minRoi: 140_000, // 14%
            maxRoi: 200_000, // 20%
            percentageMaxCvgToMint: 40,
            maxCvgToMint: ethers.parseEther((TOTAL_CVG_TO_SOLD_IBO * 0.25).toString()),
        },
    },
    CRV: {
        BOND_PARAMETERS: {
            composedFunction: 0,
            token: TOKENS.CRV.address,
            gamma: 250_000, // 0.25%
            scale: 5_000, // 0.5%
            minRoi: 140_000, // 14%
            maxRoi: 200_000, // 20%
            percentageMaxCvgToMint: 20,
            maxCvgToMint: ethers.parseEther((TOTAL_CVG_TO_SOLD_IBO * 0.5).toString()),
        },
    },
    CVX: {
        BOND_PARAMETERS: {
            composedFunction: 0,
            token: TOKENS.CVX.address,
            gamma: 250_000, // 0.25%
            scale: 5_000, // 0.5%
            minRoi: 140_000, // 14%
            maxRoi: 200_000, // 20%
            percentageMaxCvgToMint: 40,
            maxCvgToMint: ethers.parseEther((TOTAL_CVG_TO_SOLD_IBO * 0.25).toString()),
        },
    },
};

However, Ibo.sol calculates the max cvg per bond value by dividing it by 10^3. This means the actual percentageMaxCvgToMint will be 1/10 of the expected value. Causing users to send 10x more transactions to mint the amount they wanted to mint if they want to mint a big amount. This leads both lose of funds in form of gas and some users may end up minting less amount than they wanted if the total supply gets minted before they send more transactions

Proof of Concept:

Impact:

Deviation from an important behavior of the contract

Unexpectedly failing mints

Users loosing funds for gas

Users being force to send multiple transactions

Users may mint less than they expected

Recommended Mitigation:

shalbe-cvg commented 1 year ago

Hello, Thanks a lot for your attention.

This is the expected behavior, the calculation is correct. However, due to this misunderstanding we are going to add more comments on the decimals information.

We have so to consider this issue as Invalid.