Possible to drain DepositBatch, WithdrawBatch approver's token balance due improper input validation

[hats-bug-reporter]

Github username: @qckhp96565463
Twitter username: qckhp
Submission hash (on-chain): 0x6583efd5a55fc239b07453ec62c92bf82ef5f65ebc0311895c1bf1b8908321a7
Severity: high

Description:
Description
Possible to drain DepositBatch, WithdrawBatch, and EnsoHandlers approver's token balance due improper input validation via DepositBatch.multiTokenSwapAndTransfer() function call.
It's possible because of Enso's SafeEnsoShortcuts integration, it's delegate calling to any input contracts, which must not be allowed.

Attack Scenario
User A wants to deposit via DepositBatch contract.
User A approves DepositBatch contract to spend his USDT tokens.
Attacker calls the DepositBatch.multiTokenSwapAndTransfer() with a malcious calldata to transferFrom USDT balance of the victim to attacker wallet.

Attachments

1. Proof of Concept (PoC) File
   PoC attached.

2. Revised Code File (Optional)
   Recommended to add some whitelisting mechanism to the target contracts and function selectors.

Files:

• add_to_5_BatchTx.test.ts (https://hats-backend-prod.herokuapp.com/v1/files/QmV8ChEZoWuJLZqvr1fNmXanNHPiu8v5o65zP5TG4Umnnr)

[langnavina97]

Could you please provide more information about this exploit? The test case provided is failing. Please create the calldata in the test case rather than copying a bytestream so we can understand how you're attempting to steal the funds. Additionally, please avoid uploading files and instead put the details into a gist. @qckhp96565463

[qckhp96565463]

Hi @langnavina97 ,

Yeah actually I did manually fabricate that calldata, so I will try to make it generated based on the test env addresses.

So actually what is this exploit about: the drainable contracts are calling SafeEnsoShortcuts via delegatecall.

We can pass any commands to call by the safeEnsoShortcuts contract effectively we are able to pass to call for example the USDT contract's transferFrom function with the victim and attacker addresses, and as its a delegate call coming from the Vulnerable contracts the caller will be those contracts, leading to pass the approval check (because victim was approving the batch contracts before) hope it helps to understand more.

I will try to provide the fixed PoC asap.

Btw if you want for faster resolution maybe you could pass me the owner.address and depositor1.address and DepositBatch.address from your test env, so I can return you the calldata manually so you can see the PoC woking already.

[langnavina97]

Thank you @qckhp96565463
You can use any address for owner and depositor.

[qckhp96565463]

Hi, @langnavina97

Here is the pimped version: https://gist.github.com/qckhp96565463/a3ac3ee09082e7ae70e5b0ad95e85cfd

Also check the traces so you get a better view maybe.

If something still does not work, feel free to reach out.

✔ should init tokens (38ms)
SupplyBefore BigNumber { value: "0" }
owner USDT BALANCE before: BigNumber { value: "344256922" }
owner ARB BALANCE before: BigNumber { value: "413482633121575650466" }
attacker USDT BALANCE before: BigNumber { value: "0" }
attacker ARB BALANCE before: BigNumber { value: "0" }
owner USDT BALANCE after: BigNumber { value: "44256922" }
owner ARB BALANCE after: BigNumber { value: "413482633121574650466" }
attacker USDT BALANCE after: BigNumber { value: "300000000" }
attacker ARB BALANCE after: BigNumber { value: "0" }
✔ should drain depositBatch approvers tokens via delegate call (13479ms)

[aktech297]

If my understanding is correct, this is what the DepositBatch contract does.

User A approves DepositBatch contract to spend his USDT tokens. -- i think this is not correct.

User deposit tokens to the DepositBatch contract. After this all the transaction will be done on behalf of the DepositBatch contract.

After the executions, remaining funds in the DepositBatch contract is refunded.

[qckhp96565463]

i think this is not correct.

If you check the test cases you can see the user first approves the DepositBatch contract for example here:
https://github.com/hats-finance/Velvet-Capital-0x0bb0c08fd9eeaf190064f4c66f11d18182961f77/blob/main/test/Arbitrum/5_BatchTx.test.ts#L622

and during execution it pulls the tokens from the user here:
https://github.com/hats-finance/Velvet-Capital-0x0bb0c08fd9eeaf190064f4c66f11d18182961f77/blob/main/contracts/bundle/DepositBatch.sol#L40

[aktech297]

I think it's user responsibility to revoke the approval since they are approving first.

[langnavina97]

Thank you for submitting the issue. We've resolved it and pushed the changes, which can be found here: Velvet-Capital@25ef3e7

@qckhp96565463