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

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

Due to lack of a function to remove an existing bond (`bondId`), a user would still be able to deposit a bonded-token into the existing bond (`bondId`) via the ibo#`deposit()` even if the bonded-token was exploited #75

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

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

Github username: @0xmuxyz Submission hash (on-chain): 0x3645d3d8ecd33cf7dc84e94afb1d0581b535334a77de217f04a5da437094036f Severity: medium

Description: Title:\ Due to lack of a function to remove an existing bond (bondId), a user would still be able to deposit a bonded-token into the existing bond (bondId) via the ibo#deposit() even if the bonded-token was exploited

Severity:\ Medium

Description:\ Within the ibo contract, the BondParams struct would be defined and the bonded token (i.e. $CRV, $FRAX, $CVX) would be defined as a property of it like this:\ Ibo.sol#L29

    struct BondParams {
        uint8 composedFunction;
        IERC20Metadata token;  ///<-------- @audit
        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
    }

Within the ibo contract, the bondsParams storage would be defined to associate a bondId with a bond data in the form of the BondParams struct like this:\ Ibo.sol#L92

    mapping(uint256 => BondParams) public bondsParams;

When the owner create a new bond, the owner call the Ibo#createBond(). Within the Ibo#createBond(), new bond data (bondParams) would be stored into the bondsParams storage with the new bond ID (nextIdBond). By doing so, a bonded-token (i.e. $CRV, $FRAX, $CVX) would be associated with the new bondId like this:\ Ibo.sol#L120

    /**
     *  @notice Create a new bond with associated bondParams
     *  @param bondParams BondParams
     *
     */
    function createBond(BondParams calldata bondParams) external onlyOwner {
        bondsParams[nextIdBond++] = bondParams; ///<---- @audit
    }

However, within the ibo contract, there is no function to remove an existing bond (bondId).

If a bonded-token (i.e. $CRV, $FRAX, $CVX) of an existing bond (bondId) would be exploited, there is no way for the owner to stop bonding process of the existing bond (bondId).

As a result, a user would still be able to deposit a bonded-token into the existing bond (bondId) via the ibo#deposit() even if the bonded-token was exploited.\ Ibo.sol#L142-L220

NOTE: Especially, $CRV and $FRAX, which are used as a bonded-token in the Convergence Finance, would have some risks to be exploited respectively.

For example:

Recommendation:\ Within the ibo contract, consider adding a function to remove an existing bond (bondId) so that the owner to stop bonding process of the existing bond (bondId) if the bonded-token is exploited like this:

+   function removeBond(uint256 bondId) external onlyOwner {
+       delete bondsParams[bondId];
+   }

In addition to that, within the ibo#deposit(), consider adding a validation to check whether or not the destination bond (bondId) would be active to deposit a bonded-token.

    function deposit(
        uint256 tokenId,
        uint256 bondId,
        uint256 amountIn,
        uint256 amountOutMin,
        uint256 privilegeType,
        bytes32[] calldata _merkleProof
    ) external {
        require(amountIn > 0, "LTE");
        BondParams memory _bondParams = bondsParams[bondId];
+       require(_bondParams.token != address(0), "invalid bond");
        ...
0xR3vert commented 1 year ago

Hello, Thanks a lot for your attention. We can already stop a bond by changing the oracleParams, so if an exploit occurs on a bonded-token we would simply trigger this function to prevent losses. In conclusion we have so to consider this issue as invalid.