ERC20 tokens with Fee-on-transfer logic are not supported

[code423n4]

Lines of code

https://github.com/code-423n4/2022-06-putty/blob/3b6b844bc39e897bd0bbb69897f2deff12dc3893/contracts/src/PuttyV2.sol#L593 https://github.com/code-423n4/2022-06-putty/blob/3b6b844bc39e897bd0bbb69897f2deff12dc3893/contracts/src/PuttyV2.sol#L636

Vulnerability details

Some tokens in the Ethereum ecosystem apply transfer fees, such as DGX and CGT. The USDT token also has the fee feature which is disabled at the time of writing. However, PuttyV2 contract does not assumes that the amount in ERC20Asset.tokenAmount would be less than the amount received ERC20Asset.token address and this could lock the amount in the contract while the ERC20Asset.tokenAmount is less than the balance of PuttyV2 unless there are other order amount with the same ERC20Asset.token.

PoC

See @audit tag 1 - PuttyV2.sol#_transferERC20sIn()#L593

src/PuttyV2.sol:
593 function _transferERC20sIn(ERC20Asset[] memory assets, address from) internal {
594 for (uint256 i = 0; i < assets.length; i++) {
595 address token = assets[i].token;
596 uint256 tokenAmount = assets[i].tokenAmount;
597 
598: require(token.code.length > 0, "ERC20: Token is not contract");
599 require(tokenAmount > 0, "ERC20: Amount too small");
600 
601 ERC20(token).safeTransferFrom(from, address(this), tokenAmount); // @audit the amount received is less than the order amount.
602 }
603 }

2 - PuttyV2.sol#_transferERC20sOut()#L636

src/PuttyV2.sol:
636 function _transferERC20sOut(ERC20Asset[] memory assets) internal {
637 for (uint256 i = 0; i < assets.length; i++) {
638: ERC20(assets[i].token).safeTransfer(msg.sender, assets[i].tokenAmount); // @audit the amount can be less than the contract balance
639 }
640 }

Function affected:

1. PuttyV2.sol#fillOrder()
   • _transferERC20sIn#L368
   • _transferERC20sIn#L375
2. PuttyV2.sol#exercise() in
   • _transferERC20sIn
   • _transferERC20sOut
3. PuttyV2.sol#withdraw()
   • _transferERC20sOut

Recommendation

SOLUTION 1. Choose to not supporting them by adding a balance checking before and after transferring, if it's FoT token revert. SOLUTION 2. support them by adding a map of orders of assets that save actualOrderAssetAmounts.

[KenzoAgada]

Contest readme: "There are various tokens with custom implementations of how user balances are updated over time. The most common of these are fee-on-transfer and rebase tokens. Due to the complexity and cost of persistent accounting we don't intend to support these tokens."

[outdoteth]

echo what @KenzoAgada said

[HickupHH3]

dup of #21