search

hats-finance / illuminex-0x0bb4aa1f58719707405c231fcdf0b405714799cf

0 stars 1 forks source link

Users can bypass `protocolFees` in `packages/contracts/contracts/illuminex/xengine/chains/btc/wallet/VaultBitcoinWallet.sol` due to rounding issue. #23

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

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

Github username: @erictee2802 Twitter username: 0xEricTee Submission hash (on-chain): 0x66f0eebb9e44d8d9fad9e880b341f80ec49e7f53b57f26f11fcbd103c78595aa Severity: high

Description: Description

In packages/contracts/contracts/illuminex/xengine/chains/btc/wallet/VaultBitcoinWallet.sol::withdraw: 

 function withdraw(bytes memory to, uint64 amount, uint64 minReceiveAmount, bytes32 idSeed) public {
        uint64 amountAfterNetworkFee = amount - (BYTES_PER_OUTGOING_TRANSFER * satoshiPerByte);
        require(amountAfterNetworkFee >= minWithdrawalLimit, "AFL");

        uint64 protocolFees = amountAfterNetworkFee * withdrawalFee / 1000; //erictee-issue: splitting the withdrawal can bypass the fees.
        if (isExcludedFromFees[msg.sender]) {
            protocolFees = 0;
        }
        //REDACTED

Users can bypass protocolFees completely by passing amount = minWithdrawalLimit (Note: This value is set to 700 & withdrawalFee = 1 on initialization).

Attack Scenario

Users can bypass protocolFees by exploiting rounding issue, causing protocol to lose revenues.

Attachments

NA

  1. Proof of Concept (PoC) File

In the protocolFees equation:

        uint64 protocolFees = amountAfterNetworkFee * withdrawalFee / 1000; //erictee-issue: splitting the withdrawal can bypass the fees.

Variables values:

amountAfterNetworkFee = 700 withdrawalFee = 1

Therefore,

uint64 protocolFees = 700 * 1 / 1000 which will be rounded down to zero, causing loss of revenues to the protocol.

  1. Revised Code File (Optional)

Consider rounding up the protocolFees calculation to prevent revenues loss.

rotcivegaf commented 4 months ago

Non issue

0xEricTee commented 4 months ago

@rotcivegaf Disagree with the decision.

Similar issue was reported to Immunefi and was awarded Critical: https://medium.com/immunefi/the-graph-rounding-error-bugfix-review-c946ff470f65

batmanBinary commented 4 months ago

invalid,

Users can bypass protocolFees completely by passing amount = minWithdrawalLimit (Note: This value is set to 700 & withdrawalFee = 1 on initialization).

the withdraw() function will always revert if amount=minWithdrawalLimit as ,

 uint64 amountAfterNetworkFee = amount - (BYTES_PER_OUTGOING_TRANSFER * satoshiPerByte);
        require(amountAfterNetworkFee >= minWithdrawalLimit, "AFL");

here due to (BYTES_PER_OUTGOING_TRANSFER * satoshiPerByte) ,amount will be always less than minWithdrawalLimit and therefore reverts

0xEricTee commented 4 months ago 
amountAfterNetworkFee

What I meant is as long as amountAfterNetworkFee equals minWithdrawalLimit which is 700, the protocolFees will be rounded down to zero.

rotcivegaf commented 4 months ago

https://github.com/hats-finance/illuminex-0x0bb4aa1f58719707405c231fcdf0b405714799cf/issues/59#issuecomment-2211295805