VaultBitcoinWallet : Lack of slippage protection for user during deposit.

[hats-bug-reporter[bot]]

Github username: -- Twitter username: -- Submission hash (on-chain): 0x94443c740f6f7bd183a46e84bb118bb582d2ed807b0b62dceafc37037a9896b4 Severity: medium

Description: Description\

The withdraw function has one of the input argument minReceiveAmount which serves as protection from excessive fee charge by admin. It acts like the slippage protection.

Both deposit and withdrw operations charge some form for fee like protocol fee, network fee and import fee.

when we see the witdraw, the function has the protrction guard for user.

wheras the deposit function does not have this.

VaultBitcoinWallet.sol#L470-L486

        require(bytes20(_vaultScriptHash) == _keyDataToScriptHash(offchainPubKeyIndex, _keyIndex, keccak256(recoveryData)), "IR");

        uint64 protocolFees = value * depositFee / 1000;
        if (isExcludedFromFees[destination]) {
            protocolFees = 0;
        }

        uint64 importFees = ((BYTES_PER_INCOMING_TRANSFER + INPUT_EXTRA_FIXED_BYTES_FEE) * satoshiPerByte);
        protocolFees += importFees;

        uint64 valueAfterFees = value - protocolFees;

        btcToken.mint(destination, valueAfterFees);
        if ((protocolFees - importFees) > 0 && destination != REFUEL_VAULT_ADDRESS) {
            btcToken.mint(owner(), protocolFees - importFees);
        }

Impact\

User might face unexpected output during the deposit due to the excessive import fee and protocol fee.

2. Revised Code File (Optional)

As implemeted in the withdraw function, have input amount value likemindepositAmount for deposit process also.

[party-for-illuminati]

It cannot be implemented the same way here for the following reason:

minDepositAmount would have to be a part of recovery data, which hash is used to derive a BTC deposit address.

In case fees getting too high and a user would've already sent their BTC to the deposit address, this will lead to their funds stuck if we have slippage implemented here.

The solution here would be to have a contract as a feeSetter which would prevent setting too high satoshiPerByte and regulate it properly

[aktech297]

It cannot be implemented the same way here for the following reason:

minDepositAmount would have to be a part of recovery data, which hash is used to derive a BTC deposit address.

In case fees getting too high and a user would've already sent their BTC to the deposit address, this will lead to their funds stuck if we have slippage implemented here.

The solution here would be to have a contract as a feeSetter which would prevent setting too high satoshiPerByte and regulate it properly

I understood..

let me check this further if there is any better solution for this.

[aktech297]

Hi .. would like to hear the reason for rejecting this.

We yet to check this deep to suggest solution.

[aktech297]

@rotcivegaf

[party-for-illuminati]

Hi .. would like to hear the reason for rejecting this.

We yet to check this deep to suggest solution.

If there is a viable solution provided we will update the issue status

[aktech297]

Hi @party-for-illuminati

Having control over few setter would be difficult since the sathoshiperbyte and the protocol fee are dynamic.

One solution we think for this is, allow the user to send the BTC + fee. The fee is decided by the illuminati side and it should be valid value. So, the tx hash will have the BTC to be minted and the fee amount. This way the user will be aware that how much BTC will be minted. On the deposit side, just deduct this fee from tx value and mint the btc token to user.

The above design can be included in user guide and mention any deposit without fee can not be guaranteed to mint btc tokens.

[party-for-illuminati]

Hi @party-for-illuminati

Having control over few setter would be difficult since the sathoshiperbyte and the protocol fee are dynamic.

One solution we think for this is, allow the user to send the BTC + fee. The fee is decided by the illuminati side and it should be valid value. So, the tx hash will have the BTC to be minted and the fee amount. This way the user will be aware that how much BTC will be minted. On the deposit side, just deduct this fee from tx value and mint the btc token to user.

The above design can be included in user guide and mention any deposit without fee can not be guaranteed to mint btc tokens.

Do you mean including fee value into recoveryData?

[aktech297]

Hi @party-for-illuminati

Having control over few setter would be difficult since the sathoshiperbyte is dynamic.

One solution we think for this is, allow the user to send the BTC + fee. The fee is decided by the illuminati side and it should be valid value. So, the tx hash will have the BTC to be minted and the fee amount. This way use will be aware that how much BTC will be minted. On the deposit side, just deduct this fee from tx value and mint the btc token to user.

Yes.. and  deduct this value when processing deposit.

No other fee should be considered in processing.

[party-for-illuminati]

Hi @party-for-illuminati

Having control over few setter would be difficult since the sathoshiperbyte is dynamic.

One solution we think for this is, allow the user to send the BTC + fee. The fee is decided by the illuminati side and it should be valid value. So, the tx hash will have the BTC to be minted and the fee amount. This way use will be aware that how much BTC will be minted. On the deposit side, just deduct this fee from tx value and mint the btc token to user.

Yes.. and  deduct this value when processing deposit.

No other fee should be considered in processing.

Will research it a bit more and get back to you

[aktech297]

@party-for-illuminati

Hi, can you review this update the status ?

[rotcivegaf]

In my point of view there is no bug here, the valueAfterFees is built by: value - protocolFees, where protocolFees is calculated with values set by the feeSetter which is a trusted agent and also for the depositFee which is set by the owner

[aktech297]

In my point of view there is no bug here, the valueAfterFees is built by: value - protocolFees, where protocolFees is calculated with values set by the feeSetter which is a trusted agent and also for the depositFee which is set by the owner

Hi @rotcivegaf , Please check the withdraw function, it has the input minReceiveAmount by the user which will ensure the minimum expected amount. This will safeguard user during market dynamics where fee spikes.

When the fee is high and fluctuating, user might wait for sometime and then try for withdraw.

Also, if you note the fee, there are two components, protocol fee and import fee. The protocol fee have limit check but the import fee is dynamic and there is no upper limit as stated by sponsor here.

Having minimum output check for deposit will safeguard user as in withdraw. I think, sponser agreed on this and they wanted better sollution to accept this submission. we tried to provide out best here.

Note : in hats submission guidelines, the issue resolution is optional. I am not sure why sponsor rejects this by asking viable solution.

[aktech297]

Hi .. would like to hear the reason for rejecting this. We yet to check this deep to suggest solution.

If there is a viable solution provided we will update the issue status

I am not sure why this issue is invalidated without further comments from sponsor.

The reply from sponsor clearly shows that the issue is reasonable to fix. But they wanted the solution to award for this. but, as per Hats rule, solution is optional.

We tried to give solution and after that there is no response.

Lack of slippage issue awarded as valid medium historically in Hats, Code4rena, Sherlock and in other audit platforms.

I would like to review this submission once again.