_safeTransfer() is not safe, silent failures are possible.

Summary

_safeTransfer() is not safe, silent failures are possible.

The main problem is that it lacks two consideration: 1) the token address might be invalid - the contract does not exist; 2) it fails to check the returned bool value of the transfer function. Some ERC20 tokens will return false without revert.

Vulnerability Detail

_safeTransfer() allows a user to transfer ERC29 tokens to another user.

https://github.com/sherlock-audit/2023-04-splits/blob/main/splits-utils/src/TokenUtils.sol#L24-L27

Unfortunately, it is not safe in two aspects.

1) it does not check whether token is a contract address. In the case that the address is invalid, there will be a silent failure.

The following POC code confirms my finding:

function testSafeTransfer() public {
        address invalidToken = address(1234);
        vm.expectRevert();
        _safeTransfer(invalidToken, address(2), 1000);
    }

The test will fail, meaning there is no revert, a silent failure!

2) When the target ERC20 token does not revert but return false during transfer failure (not enough balance), the function does not check whether the returned value is true or false and simply assume the transfer is successful in this case.

The following POC code confirms my finding, a fake token is created for the testing:

   function testSafeTransferReturnFalse() public {
          TokenA dummy = new TokenA();

          vm.expectRevert();
         _safeTransfer(address(dummy), address(2), 1000);

    }

pragma solidity 0.8.17;

contract TokenA{

    function transfer(address to, uint rawAmt) public returns (bool success) {
          return false;
    }
}

Again, the test fails, meaning there is no revert and a silent failure just occurred.

Impact

Silent failures might occur due to failing to check the existence of the token contract and the returned bool value of the transfer function.

Code Snippet

See above

Tool used

VSCode

Manual Review

Recommendation

Use Openzepplin's safeTransferLib library instead.

sherlock-audit / 2023-04-splits-judging

chaduke - _safeTransfer() is not safe, silent failures are possible. #67