missing !claimed check in NATIVE claimWithdrawal() function

[hats-bug-reporter]

Github username: --
Twitter username: --
Submission hash (on-chain): 0xabd55c24fa6ef983a1bb8b977bcaea6590d1546788a87a48102e26a0cc1d3095
Severity: high

Description:
Description
in the contract minter.sol and contract NativeMinterWithdrawal and claimwithdrawal L2126 which is going to claim the withdrawals with native token

  function claimWithdrawal(uint256 withdrawalId, address receiver) public virtual nonReentrant {
        require(ownerOf(withdrawalId) == msg.sender, "NotOwner");
        WithdrawalRequest storage request = _withdrawalRequests[withdrawalId];
        require(request.processed, "NotProcessedYet");
        _burn(withdrawalId);
        request.claimed = true;
        totalUnclaimedWithdrawals = totalUnclaimedWithdrawals-request.amount;
        SafeTransferLib.safeTransferETH(receiver, request.amount);
        emit ClaimWithdrawal(address(msg.sender), receiver, request.amount, withdrawalId);
    }

sets the status to claimed but doesnt check that doesnt actually require that status should be !Claimed

like in the L2170 claimWithdrawal erc20 which DOES check that it should not be claimed but native one doesnt
erc20 one: require(!request.claimed, "AlreadyClaimed");
as you see its fixed in the erc20 one but in the native one issue STILL REMAINS

Recommendation

• consider modifying function to the way that it actually checks user claimed or not otherwise its possibe to reclaim

[0xRizwan]

Agreed, it should be checked but it wont lead to any vulnerability as When the user claims his withdrawal, the withdrawalId will burn i.e the withdrawalId owner would be address(0) now. when the user will try again to claim withdrawal the function will revert with "NotOwner".

Would leave on sponsor to decide its validity, imo it should be informational.

[0xarshia]

hey Rizwan thanks for your Comment,

i double check it this will not cause loss of funds however i think it should be at least Low severity cause due to terms of the Hats Platform

Issues where the behavior of the contracts differs from the intended behavior (as described in the docs and by common sense), but no funds are at risk.

 function claimWithdrawal(uint256 withdrawalId, address receiver) public virtual nonReentrant {
        require(ownerOf(withdrawalId) == msg.sender, "NotOwner");
        WithdrawalRequest storage request = _withdrawalRequests[withdrawalId];
        require(request.processed, "NotProcessedYet");
        require(!request.claimed, "AlreadyClaimed");
        _burn(withdrawalId);
        request.claimed = true;
        totalUnclaimedWithdrawals = totalUnclaimedWithdrawals-request.amount;
        baseToken.safeTransfer(receiver, request.amount);
        emit ClaimWithdrawal(address(msg.sender), receiver, request.amount, withdrawalId);
    }

as you see the check is being made in L2170 but not in NATIVE one

Fix of the issue in Native claim

  function claimWithdrawal(uint256 withdrawalId, address receiver) public virtual nonReentrant {
        require(ownerOf(withdrawalId) == msg.sender, "NotOwner");
        WithdrawalRequest storage request = _withdrawalRequests[withdrawalId];
        require(request.processed, "NotProcessedYet");
+       require(!request.claimed, "AlreadyClaimed");
        _burn(withdrawalId);
        request.claimed = true;
        totalUnclaimedWithdrawals = totalUnclaimedWithdrawals-request.amount;
        SafeTransferLib.safeTransferETH(receiver, request.amount);
        emit ClaimWithdrawal(address(msg.sender), receiver, request.amount, withdrawalId);
    }

Thanks

[Slavchew]

That was removed after their first audit since has no actual value, just an unnecessary double check - AccumulatedFinance/contracts-v2@9fae5f3

[0xRizwan]

That was removed after their first audit since has no actual value, just an unnecessary double check - AccumulatedFinance/contracts-v2@9fae5f3

Thanks for sharing. request.claim is purposely not checked by protocol team so marking this issue as invalid.

[0xarshia]

yes this was removed in the commit i just saw it today but then it should be deleted from this function also so i believe it's at least low let's see sponsors' point of view

removing this and not removing another one doesn't any makes sense at all

ERC20 claim which has this require and hasn't been removed since last audit

function claimWithdrawal(uint256 withdrawalId, address receiver) public virtual nonReentrant {
        require(ownerOf(withdrawalId) == msg.sender, "NotOwner");
        WithdrawalRequest storage request = _withdrawalRequests[withdrawalId];
        require(request.processed, "NotProcessedYet");
        require(!request.claimed, "AlreadyClaimed");
        _burn(withdrawalId);
        request.claimed = true;
        totalUnclaimedWithdrawals = totalUnclaimedWithdrawals-request.amount;
        baseToken.safeTransfer(receiver, request.amount);
        emit ClaimWithdrawal(address(msg.sender), receiver, request.amount, withdrawalId);
    }

whats your opinion @ilzheev

[ilzheev]

ERC20 claim which has this require and hasn't been removed since last audit

Yes, it's redundant check in ERC20Minter.
I marked it as low.

[ilzheev]

@0xRizwan AccumulatedFinance/contracts-v2@4ed4034