Limited support to a specific subset of ERC20 tokens
Buffer contest states 'any ERC20 supported', therefore it should take into account all the different ways of signalling success and failure. This is not the case, as all ERC20's transfer(), transferFrom(), and approve() functions are either not verified at all or verified for returning true. As a result, depending on the ERC20 token, some transfer errors may result in passing unnoticed, and/or some successfull transfer may be treated as failed.\\nCurrently the only supported ERC20 tokens are the ones that fulfill both the following requirements:\\nalways revert on failure;\\nalways returns boolean true on success.\\nAn example of a very well known token that is not supported is Tether USD (USDT).\\n👋 IMPORTANT This issue is not the same as reporting that "return value must be verified to be true" where the checks are missing! Indeed such a simplistic report should be considered invalid as it still does not solve all the problems but rather introduces others. See Vulnerability Details section for rationale.\\nTokens have different ways of signalling success and failure, and this affect mostly transfer(), transferFrom() and approve() in ERC20 tokens. While some tokens revert upon failure, others consistently return boolean flags to indicate success or failure, and many others have mixed behaviours.\\nSee below a snippet of the USDT Token contract compared to the 0x's ZRX Token contract where the USDT Token transfer function does not even return a boolean value, while the ZRX token consistently returns boolean value hence returning false on failure instead of reverting.\\nUSDT Token snippet (no return value) from Etherscan\\n```\\nfunction transferFrom(address _from, address _to, uint _value) public onlyPayloadSize(3 * 32) {\\n var _allowance = allowed[_from][msg.sender];\\n\\n // Check is not needed because sub(_allowance, _value) will already throw if this condition is not met\\n // if (_value > _allowance) throw;\\n\\n uint fee = (_value.mul(basisPointsRate)).div(10000);\\n if (fee > maximumFee) {\\n  fee = maximumFee;\\n }\\n if (_allowance < MAX_UINT) {\\n  allowed[_from][msg.sender] = _allowance.sub(_value);\\n }\\n uint sendAmount = _value.sub(fee);\\n balances[_from] = balances[_from].sub(_value);\\n balances[_to] = balances[_to].add(sendAmount);\\n if (fee > 0) {\\n  balances[owner] = balances[owner].add(fee);\\n  Transfer(_from, owner, fee);\\n }\\n Transfer(_from, _to, sendAmount);\\n}\\n```\\n\\nZRX Token snippet (consistently true or false boolean result) from Etherscan\\n```\\nfunction transferFrom(address _from, address _to, uint _value) returns (bool) {\\n if (balances[_from] >= _value && allowed[_from][msg.sender] >= _value && balances[_to] + _value >= balances[_to]) {\\n  balances[_to] += _value;\\n  balances[_from] -= _value;\\n  allowed[_from][msg.sender] -= _value;\\n  Transfer(_from, _to, _value);\\n  return true;\\n } else { return false; }\\n}\\n```\\n
To handle most of these inconsistent behaviors across multiple tokens, either use OpenZeppelin's SafeERC20 library, or use a more reusable implementation (i.e. library) of the following intentionally explicit, descriptive example code for an ERC20 transferFrom() call that takes into account all the different ways of signalling success and failure, and apply to all ERC20 transfer(), transferFrom(), approve() calls in the Buffer contracts.\\n```\\nIERC20 token = whatever_token;\\n\\n(bool success, bytes memory returndata) = address(token).call(abi.encodeWithSelector(IERC20.transferFrom.selector, sender, recipient, amount));\\n\\n// if success == false, without any doubts there was an error and callee reverted\\nrequire(success, "Transfer failed!");\\n\\n// if success == true, we need to check whether we got a return value or not (like in the case of USDT)\\nif (returndata.length > 0) {\\n // we got a return value, it must be a boolean and it should be true\\n require(abi.decode(returndata, (bool)), "Transfer failed!");\\n} else {\\n // since we got no return value it can be one of two cases:\\n // 1. the transferFrom does not return a boolean and it did succeed\\n // 2. the token address is not a contract address therefore call() always return success = true as per EVM design\\n // To discriminate between 1 and 2, we need to check if the address actually points to a contract\\n require(address(token).code.length > 0, "Not a token address!");\\n}\\n```\\n
Given the different usages of token transfers in BufferBinaryOptions.sol, BufferBinaryPool.sol, and BufferRouter.sol, there can be 2 types of impacts depending on the ERC20 contract being traded.\\nThe ERC20 token being traded is one that consistently returns a boolean result in the case of success and failure like for example 0x's ZRX Token contract. Where the return value is currently not verified to be true (i.e.: #1, #2, #3, #4, #5, #6) the transfer may fail (e.g.: no tokens transferred due to insufficient balance) but the error would not be detected by the Buffer contracts.\\nThe ERC20 token being traded is one that do not return a boolean value like for example the well knonw Tether USD Token contract. Successful transfers would cause a revert in the Buffer contracts where the return value is verified to be true (i.e.: #1, #2, #3, #4) due to the token not returing boolean results.\\nSame is true for appove calls.
```\\nfunction transferFrom(address _from, address _to, uint _value) public onlyPayloadSize(3 * 32) {\\n var _allowance = allowed[_from][msg.sender];\\n\\n // Check is not needed because sub(_allowance, _value) will already throw if this condition is not met\\n // if (_value > _allowance) throw;\\n\\n uint fee = (_value.mul(basisPointsRate)).div(10000);\\n if (fee > maximumFee) {\\n  fee = maximumFee;\\n }\\n if (_allowance < MAX_UINT) {\\n  allowed[_from][msg.sender] = _allowance.sub(_value);\\n }\\n uint sendAmount = _value.sub(fee);\\n balances[_from] = balances[_from].sub(_value);\\n balances[_to] = balances[_to].add(sendAmount);\\n if (fee > 0) {\\n  balances[owner] = balances[owner].add(fee);\\n  Transfer(_from, owner, fee);\\n }\\n Transfer(_from, _to, sendAmount);\\n}\\n```\\n