MohammedRizwan - Possible loss of funds, transfer functions can silently fail

[sherlock-admin2]

MohammedRizwan

Medium

Possible loss of funds, transfer functions can silently fail

Summary

Possible loss of funds, transfer functions can silently fail

Vulnerability Detail

Union Protocol's contracts are expected to be used USDT, USDC and DAI. The contracts will be deployed on Any EVM compatible chain which also includes Ethereum mainnet itself. Both of these details are mentioned in contest readme. This issue is specifically for tokens like USDT and similar tokens etc on Ethereum mainnet.

The following functions makes use of ERC20's transferFrom() in following contracts:

1) In VouchFaucet.sol, the claimTokens() is called by users to claim the tokens and the token is transferred to user but the transfer return value is not checked and similarly in case of transferERC20() function.

    function claimTokens(address token, uint256 amount) external {
        require(claimedTokens[token][msg.sender] <= maxClaimable[token], "amount>max");
@>        IERC20(token).transfer(msg.sender, amount);     @audit // unchecked transfer return value 
        emit TokensClaimed(msg.sender, token, amount);
    }

    function transferERC20(address token, address to, uint256 amount) external onlyOwner {
@>        IERC20(token).transfer(to, amount);                @audit // unchecked transfer return value 
    }

2) In ERC1155Voucher.transferERC20(), tokens are being transferred to recipient address and return value is not checked.

    function transferERC20(address token, address to, uint256 amount) external onlyOwner {
@>        IERC20(token).transfer(to, amount);            @audit // unchecked transfer return value 
    }

The issue here is with the use of unsafe transfer() function. The ERC20.transfer() function return a boolean value indicating success. This parameter needs to be checked for success. Some tokens do not revert if the transfer failed but return false instead.

Some tokens like USDT don't correctly implement the EIP20 standard and their transfer() function return void instead of a success boolean. Calling these functions with the correct EIP20 function signatures will always revert.

Tokens that don't actually perform the transfer and return false are still counted as a correct transfer and tokens that don't correctly implement the latest EIP20 spec, like USDT, will be unusable in the protocol as they revert the transaction because of the missing return value. There could be silent failure in transfer which may lead to loss of user funds in ERC1155Voucher.transferERC20() and VouchFaucet.claimTokens()

Impact

Tokens that don't actually perform the transfer and return false are still counted as a correct transfer and tokens that don't correctly implement the latest EIP20 spec will be unusable in the protocol as they revert the transaction because of the missing return value. This will lead to loss of user funds.

Code Snippet

https://github.com/sherlock-audit/2024-06-union-finance-update-2/blob/main/union-v2-contracts/contracts/peripheral/VouchFaucet.sol#L95

https://github.com/sherlock-audit/2024-06-union-finance-update-2/blob/main/union-v2-contracts/contracts/peripheral/VouchFaucet.sol#L124

Tool used

Manual Review

Recommendation

Use OpenZeppelin's SafeERC20 versions with the safeTransfer() function instead of transfer().

For example, consider below changes in VouchFaucet.sol:

+ import "@openzeppelin/contracts/token/ERC20/utils/SafeERC20.sol";

contract VouchFaucet is Ownable {

+      using SafeERC20 for IERC20;

    function claimTokens(address token, uint256 amount) external {
        require(claimedTokens[token][msg.sender] <= maxClaimable[token], "amount>max");
-        IERC20(token).transfer(msg.sender, amount);     
+       IERC20(token).safeTransfer(msg.sender, amount);    
        emit TokensClaimed(msg.sender, token, amount);
    }

    function transferERC20(address token, address to, uint256 amount) external onlyOwner {
-        IERC20(token).transfer(to, amount);              
+        IERC20(token).safeTransfer(to, amount);    
    }

[sherlock-admin3]

1 comment(s) were left on this issue during the judging contest.

0xmystery commented:

Low QA on safeTransfer

[0xRizwan]

I believe, this issue is incorrectly judged.

@mystery0x

Low QA on safeTransfer

There is no such rule at sherlock.

Per the contest readme, the protocol is expected to be deployed on any EVM compatible network so this includes Ethereum mainnet itself and the above issue regarding safe transfers is relevant for Ethereum mainnet only.

On what chains are the smart contracts going to be deployed? Any EVM compatible network

contest readme further confirms, USDT/USDC would be used in contracts.

If you are integrating tokens, are you allowing only whitelisted tokens to work with the codebase or any complying with the standard? Are they assumed to have certain properties, e.g. be non-reentrant? Are there any types of weird tokens you want to integrate? USDC, USDT, DAI

Per sherlock rules,

23. Non-Standard tokens: Issues related to tokens with non-standard behaviors, such as weird-tokens are not considered valid by default unless these tokens are explicitly mentioned in the README.

Since, USDT is mentioned in readme so the non-standard behabiour pertaining to USDT would be valid issue for this contest.

Affected functions would be transferERC20() in VouchFaucet.sol and ERC1155Voucher.sol.

    function transferERC20(address token, address to, uint256 amount) external onlyOwner {
        IERC20(token).transfer(to, amount);
    }

Similar such issue with similar readme questionare had been judged as Medium severity. see- Notional issue and Teller

Therefore, based on above arguements this issue should be valid medium severity.

[0xMR0]

Escalate

on behalf of @0xRizwan

[sherlock-admin3]

Escalate

on behalf of @0xRizwan

You've created a valid escalation!

To remove the escalation from consideration: Delete your comment.

You may delete or edit your escalation comment anytime before the 48-hour escalation window closes. After that, the escalation becomes final.

[mystery0x]

This is a widely known issue typically deemed low/informational. Will let the sponsors and the Sherlock judge decide its medium validity.

[WangSecurity]

Just checking for transfer return value is invalid I agree, but the problem here is that regular transfer will try to extract a boolean from the transfer which will revert, so for example, the users won't be able to call claimTokens in VouchFacet. But, regular transfer is used only in VouchFacet and ERC1155Voucher, @mystery0x and @0xRizwan could you assist me with how severe these would be the revert in these functions, and what operations would be impossible in case of such reverts?

[Bauchibred]

HI @WangSecurity, to chime in here, wouldn't the fact that these contracts do not work with USDT suffice as medium, since it's been indicated in the readMe that the protocol is to work with USDT, USDC & DAI? If yes, do check #17, I was initially waiting till the resolution of this escalation before hinting it's a duplicate. But I believe both reports are duplicates and #17 does show how ERC1155Voucher#transferERC20() & VouchFaucet#transferERC20() would never work with USDT.

[WangSecurity]

@Bauchibred fair point, still I wanted to know if maybe there's no impact and these functions wouldn't be used. But further considering this, I agree it's medium severity, since regardless of the goal of these functions, they won't work with USDT. Planning to accept the escalation. The duplicate is #17, @mystery0x @Bauchibred @0xRizwan are there any other duplicates?

[MD-YashShah1923]

@WangSecurity https://github.com/sherlock-audit/2024-06-union-finance-update-2-judging/issues/116 this as well is duplicate of this issue.

[0xRizwan]

@WangSecurity #11 #17 #57 #116 should be duplicates.

[WangSecurity]

Result: Medium Has duplicates

PS: I see some of the duplicates are insufficient, I'll comment on them directly explaining why.

[sherlock-admin4]

Escalations have been resolved successfully!

Escalation status:

• 0xMR0: accepted