hats-finance / Tapioca-0xe0b920d38a0900af3bab7ff0ca0af554129f54ad

4 stars 4 forks source link

`AirdropBroker.sol`: participants can claim their eligible amount as many times as possible #18

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

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

Github username: @Emedudu Twitter username: -- Submission hash (on-chain): 0x8cbb31b6b61e73f909da1bbf1d068dd2b8204b6bc8b8dec33ebb0dbb1d7277f8 Severity: high

Description: Description\ Since epoch is used in aoTAPCalls mapping, participants will be able to withdraw their eligible amount each time a new epoch starts.

Attack Scenario\ When a user(who was prelisted for the first phase) calls AirdropBroker#participate, _participatePhase1 gets called:

function _participatePhase1() internal returns (uint256 oTAPTokenID) {
    uint256 _eligibleAmount = phase1Users[msg.sender];
    if (_eligibleAmount == 0) revert NotEligible();

    // Close eligibility
    phase1Users[msg.sender] = 0;

    // Mint aoTAP
    uint128 expiry = uint128(lastEpochUpdate + EPOCH_DURATION); // Set expiry to the end of the epoch
    oTAPTokenID = aoTAP.mint(
        msg.sender,
        expiry,
        uint128(PHASE_1_DISCOUNT),
        _eligibleAmount,
        epoch
    );
}

An aoTAP token, which contains the eligible amount to claim, is minted to the user.

User can immediately call AirdropBroker#exerciseOption to claim the "eligible amount" for that his aoTAP tokenID.

The issue is that the user can claim this "eligible amount" each time a new epoch starts for as many times as possible. This is because of what is done in exerciseOption function:

   function exerciseOption(
        uint256 _aoTAPTokenID,
        ERC20 _paymentToken,
        uint256 _tapAmount
    ) external whenNotPaused tapExists {
        ...

        uint256 cachedEpoch = epoch;

        ...
        uint256 eligibleTapAmount = aoTapOption.amount;
        eligibleTapAmount -= aoTAPCalls[_aoTAPTokenID][cachedEpoch]; // Subtract already exercised amount@audit-info users can claim 4*eligibleTapamount
        if (eligibleTapAmount < _tapAmount) revert TooHigh();

        uint256 chosenAmount = _tapAmount == 0 ? eligibleTapAmount : _tapAmount;
        if (chosenAmount < 1e18) revert TooLow();
        aoTAPCalls[_aoTAPTokenID][cachedEpoch] += chosenAmount;

        ...

    }

Since epoch is included in the aoTAPCalls mapping, participants wiil be able to claim the full eligible amount each time a new epoch starts.

This may also cause payment issues for participants of later phases as earlier participants have claimed more than they should.

Attachments

  1. Proof of Concept (PoC) File

  2. Revised Code File (Optional) cachedEpoch should not be used in the aoTAPCalls mapping

0xRektora commented 5 months ago

Correct me If I'm wrong, but that's how it's supposed to work. Each epoch, a user gets his new aoTAP with his eligible amount in participate(). The user can exercise to his convenience with partial amounts, the logic you mentioned above is just logic accounting to keep track of what was previously exercised.