code423n4 commented 1 year ago

Lines of code

https://github.com/code-423n4/2022-12-Stealth-Project/blob/main/router-v1/contracts/libraries/TransferHelper.sol#L23

Vulnerability details

Impact

The safeTransfer function does not check for the existence of the ERC20 token contract , TransferHelper.sol performs a transfer with a low-level call without confirming the contract's existence

router-v1/contracts/libraries/TransferHelper.sol:
  22      /// @param value The value of the transfer
  23:     function safeTransfer(address token, address to, uint256 value) internal {
  24:         (bool success, bytes memory data) = token.call(abi.encodeWithSelector(IERC20.transfer.selector, to, value));
  25:         require(success && (data.length == 0 || abi.decode(data, (bool))), "ST");
  26:     }

The low-level functions call, delegatecall and staticcall return true as their first return value if the account called is non-existent, as part of the design of the EVM. Account existence must be checked prior to calling if needed.

It allows malicious people to pair with a qualified token like ETH with shit tokens that they can destroy later, and most importantly, to run the safeTransfer function even if the token contract is later destroyed.

Proof of Concept

1 Alice creates a pair of A and B Tokens (For exampleETH - TestToken Pair) The creator and only owner of TestToken is Alice.

2 Next, Alice destroys the TestToken with a Selfdestruct based on onlyowner privilege.

3 Bob, unaware of this, deposits ETH into the pool to trade from the ETH-TestToken pair, but cannot receive any tokens because SafeTransfer does not check for the existence of the contract

Similar vulnerability and classification; https://github.com/code-423n4/2022-09-quickswap-findings/issues/86

On January 28, QBridge was hacked and asset values of around 80 million USD were stolen. After analysis, found that the root cause in the code is the implementation of the safeTransfer(and safeTransferFrom) function;

https://blocksecteam.medium.com/when-safetransfer-becomes-unsafe-lesson-from-the-qbridge-security-incident-c32ecd3ce9da


// POC
// The execution of TestSafeTransfer.test() will not revert:
pragma solidity ^0.8.0;

contract TestSafeTransfer{
    function safeTransferFrom(
        address token,
        address from,
        address to,
        uint value
    ) internal {
        // bytes4(keccak256(bytes('tansferFrom(address,address,uint256)')));
        (bool success, bytes memory data) = token.call(abi.encodeWithSelector
        (0x23b872dd, from, to, value));
        require(success && (data.length == 0 || abi.decode(data, (bool))),
        "!safeTransferFrom");
    }

    function test() external {
        safeTransferFrom(address(0), msg.sender,msg.sender,0);
    }
}


Running 1 test for src/test/test.sol:POC
[PASS] test() (gas: 8715)
Traces:
  [8715] POC::test() 
    ├─ [3524] TestSafeTransfer::test() 
    │   ├─ [0] 0x0000000000000000000000000000000000000000:
:23b872dd(0000000000000000000000007fa9385be102ac3eac297483dd6233d62b3e1496
0000000000000000000000007fa9385be102ac3eac297483dd6233d62b3e1496
0000000000000000000000000000000000000000000000000000000000000000) 
    │   │   └─ ← ()
    │   └─ ← ()
    └─ ← ()

Test result: ok. 1 passed; 0 failed; finished in 175.17µs

Tools Used

Manual code review

Recommended Mitigation Steps

Have the SafeTransfer function check the existence of the contract before every transaction

c4-judge commented 1 year ago

kirk-baird marked the issue as primary issue

gte620v commented 1 year ago

Disagree that this is an issue to be addressed. If a user controls a malicious token contract and then creates a pool with that token, there are a limitless number of attacks. i.e. they could transfer/burn/mint tokens from their contract at will.

c4-sponsor commented 1 year ago

gte620v marked the issue as sponsor disputed

kirk-baird commented 1 year ago

While I appreciate this has been rated as Medium in the past in quickswap 86 I don't consider this to be a valid medium.

There are a few reasons I think the case where self-destructible tokens exist are so niche that it would only be malicious tokens that may use this attack vector and it is trivial to make a pool with a malicious token.

Futhermore, there is active development to remove SELFDESTRUCT opcode from the EVM and it's likely to be implemented in the next hard fork. See EIP 4758 for more details.

However, I do recommend adding the check for contract existence when performing safe transfer and will downgrade this to QA.