hats-finance / Intuition-0x538dbadc50cc87b281cd655f1edbc6ebda02a66a

The smart contracts of the Intuition protocol v1.
https://intuition.systems
Other
0 stars 1 forks source link

EthMultiVault doesn't refund excess ETH sent by the users which will result in ethers getting stuck in the contract #13

Open hats-bug-reporter[bot] opened 3 months ago

hats-bug-reporter[bot] commented 3 months ago

Github username: @ShaheenRehman Twitter username: 0x_Shaheen Submission hash (on-chain): 0x57ccad036533feabb044359e7891a16783e609b47524cff3d350f51e4df3d0d0 Severity: low

Description: Description\ EthMultiVault's batch functions make sure that enough ETH is sent by the user but it doesn't strictly make sure that nor it refunds the excess ethers to the user which will result in ethers getting stuck in the contract as the function have no functionalit7y to rescue frozen tokens.

Attack Scenario\ EthMultiVault's batchCreateAtom() & batchCreateTriple() allows users to create atoms or triples in a batch. For simplicity lets only examine the batchCreateAtom function:

    function batchCreateAtom(bytes[] calldata atomUris) external payable nonReentrant whenNotPaused returns (uint256[] memory){
        uint256 length = atomUris.length;
        ///@audit doesn't refunds the excessive fee
        if (msg.value < getAtomCost() * length) {
            revert Errors.MultiVault_InsufficientBalance();
        }

        uint256 valuePerAtom = msg.value / length;
        uint256 protocolDepositFeeTotal;
        uint256[] memory ids = new uint256[](length);

        for (uint256 i = 0; i < length; i++) {
            uint256 protocolDepositFee;
            (ids[i], protocolDepositFee) = _createAtom(atomUris[i], valuePerAtom);

            // add protocol deposit fees to total
            protocolDepositFeeTotal += protocolDepositFee;
        }

        uint256 totalFeesForProtocol = atomConfig.atomCreationProtocolFee * length + protocolDepositFeeTotal;
        _transferFeesToProtocolVault(totalFeesForProtocol);

        return ids;
    }

As we can see the function makes sure that the user sends enough msg.value:

        if (msg.value < getAtomCost() * length) {
            revert Errors.MultiVault_InsufficientBalance();
        }

But a user can send more than enough msg.value, accidentaly or intentionally to make sure that the trx get successfully submitted. When this will happen, the user will loose the excess sent ethers because there is no refund functionality and no rescue tokens functionality, the excess ethers will be freezed in the contract.

Recomended Mitigation\ There are 2 solutions to this issue:

  1. Refund the excess fee after atoms/triple creations and transfer of the protocol fee
  2. Add a strict msg.value check:
        if (msg.value == getAtomCost() * length) {
            revert Errors.MultiVault_InsufficientBalance();
        }
mihailo-maksa commented 3 months ago

This issue has been reviewed, and we have determined it to be invalid. Here are the reasons:

  1. Intended Design: The current design ensures that any excess msg.value beyond the required minimums results in additional shares being minted for the user in the vault. This is intentional and aligns with our protocol's design to incentivize higher contributions by providing proportional shares.
  2. User Benefits: Users are not losing their excess ETH; instead, they are receiving additional shares in the vault, which equates to proportional ownership and benefits within the vault.
  3. Functionality Assurance: The functionality works as intended, ensuring users always receive value for their contributions. There is no ether being "stuck" as it is translated into shares.

In conclusion, the current implementation incentivizes user participation by converting excess msg.value into additional shares. This design is intentional, and no changes are required.