Protocol will not work with tokens that do not return bool value on approve calls

[hats-bug-reporter[bot]]

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

Description: Description\ The protocol is intended to work with any ERC20 compatible tokens and even with some incompatible i.e. tokens that do not return a bool value when calling some of their functions such as transfer or transferFrom directly. This is achieved by utilizing low-level calls within the transfer functions implemented in the protocol. However within the BfxVault.sol contract we have the makeDeposit() function that is callable only by users with TREASURER_ROLE. This function calls an internal _doDeposit() function which executes two steps:

1. Approves the BFX contract to spend a desired amount
2. Invokes the deposit() function on the BFX contract which transfers the provided amount from the BfxVault contract.

In order for the execution to happen properly the TRASURER has to transfer the specified amount before invoking the makeDeposit() function.

The vulnerability here presents itself during the execution of makeDeposit() where a call is made to paymentToken.approve(), where paymentToken is wrapped with the IERC20 interface. However for tokens that do not return a bool value when calling approve such as USDT on Ethereum, the transaction will revert with an EVM error.

I am considering this issue with Medium severity due to the possibility of the owner to retrieve those tokens by invoking withdrawTokensTo().

Attack Scenario\

If paymentToken is set to USDT or any other token that do not return a bool value when approve() function is called, the TREASURER of the BfxVault will not be able to make a deposit from the vault to the Bfx Exchange due to failure in the makeDeposit() function execution.

Attachments

1. Proof of Concept (PoC) File The BfxVault.t.sol is the PoC for the discovered issue. Steps to run it:
   • Add the file to the test folder
   • Add an RPC url as a local env variable called ETH_RPC_URL
   • Run the test with forge test --match-contract BfxFail --fork-url $ETH_RPC_URL -vv
   • You should see a failed test with EvmError: Revert message
2. Revised Code File (Optional)
   • Added a _makeApprove internal function which utilizes low-level calls similar to _makeTransfer and _makeTransferFrom
   • Optional: You can also add a _makeApprove call to set the approval to 0 in order to avoid future issues with tokens that require approval to 0 first

Files:

• BfxVault.t.sol (https://hats-backend-prod.herokuapp.com/v1/files/QmZR6FfhbRNn99KFkdnxc99p8udX2BLZkRGGatgbWiz6S1)
• BfxVault.sol (https://hats-backend-prod.herokuapp.com/v1/files/QmciFNcsN8nq4kjJ1CZYaviJV3B73xrrV3JMC5XK9Nen7q)

[alex-sumner]

No attempt is made to use the return value from approve.

[Neverland0xy]

@alex-sumner The token is wrapped in IERC20 interface. However if the token does not return a bool value for the approve transaction, the entire transaction fails, because the IERC20 interface tries to return a bool when the approve call is made, but there is nothing to return so the call fails.

The current test is not catching it, because it is using the DummyToken which is ERC20 compatible. The provided test uses directly USDT (a non-compatible ERC20 token) on a mainnet fork for simplicity. However this issue could arise with any non-ERC20 compatible token on any chain.

[alex-sumner]

Are you sure you attached the correct BfxVault.t.sol ? The one above seems identical to the original.

[alex-sumner]

@alex-sumner The token is wrapped in IERC20 interface. However if the token does not return a bool value for the approve transaction, the entire transaction fails, because the IERC20 interface tries to return a bool when the approve call is made, but there is nothing to return so the call fails.

The current test is not catching it, because it is using the DummyToken which is ERC20 compatible. The provided test uses directly USDT (a non-compatible ERC20 token) on a mainnet fork for simplicity. However this issue could arise with any non-ERC20 compatible token on any chain.

Apologies, you are right. I reproduced it by making DummyToken non-compliant. I don't think it is medium however as it can only happen if the deployer, or subsequently the owner, uses a non-compliant token. So it falls under either a misconfigured deployment that will never work, or a malicious owner.

[Neverland0xy]

@alex-sumner Although I agree that it is related to misconfigured deployment or a malicious owner / mistake by owner, I think applying the suggested fix does not hurt as it is quite similar to the approach taken for transfer and transferFrom. Based on your input I would still consider it a Low validity issue, tho I am not sure what base fee receiver labeled - low means.

[alex-sumner]

Yes I agree that switching to a low level call as is done for transfer makes sense. I have changed the labelling to indicate a valid report of a low level vulnerability, and I have removed the "won't fix" label.

[Neverland0xy]

Btw applying the fix will also help the owner not having to check each paymentToken contract to see if it is ERC20 compliant :)