code423n4 commented 2 years ago

Lines of code

https://github.com/code-423n4/2022-06-putty/blob/3b6b844bc39e897bd0bbb69897f2deff12dc3893/contracts/src/PuttyV2.sol#L636-L640 https://github.com/code-423n4/2022-06-putty/blob/3b6b844bc39e897bd0bbb69897f2deff12dc3893/contracts/src/PuttyV2.sol#L646-L650 https://github.com/code-423n4/2022-06-putty/blob/3b6b844bc39e897bd0bbb69897f2deff12dc3893/contracts/src/PuttyV2.sol#L657-L661

Vulnerability details

Impact

There are no bounds on the number of tokens transferred in an order, and gas requirements can change (especially since orders can have a duration of 27 years), so orders filled at time T1 may not be exercisable/withdrawable at time T2, or with the provided assets if the assets use a lot of gas during their transfers (e.g. aTokens and cTokens). The buyer of the option will have paid the premium, and will be unable to get the assets they are owed.

Proof of Concept

There are no upper bounds on the number of assets being transferred in these loops:

File: contracts/src/PuttyV2.sol   #1

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);
639           }
640       }

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

File: contracts/src/PuttyV2.sol   #2

646       function _transferERC721sOut(ERC721Asset[] memory assets) internal {
647           for (uint256 i = 0; i < assets.length; i++) {
648               ERC721(assets[i].token).safeTransferFrom(address(this), msg.sender, assets[i].tokenId);
649           }
650       }

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

File: contracts/src/PuttyV2.sol   #3

657       function _transferFloorsOut(address[] memory floorTokens, uint256[] memory floorTokenIds) internal {
658           for (uint256 i = 0; i < floorTokens.length; i++) {
659               ERC721(floorTokens[i]).safeTransferFrom(address(this), msg.sender, floorTokenIds[i]);
660           }
661       }

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

Tools Used

Code inspection

Recommended Mitigation Steps

Have an upper bound on the number of assets, or allow them to be transferred out one at a time, if necessary

outdoteth commented 2 years ago

Adding a hardcoded check at the contract level is not a viable fix given that gas costs and limits are subject change over time. Instead, there already exists a limit of 30 assets on the frontend/db level.

outdoteth commented 2 years ago

Report: Unbounded loop can prevent put option from being exercised

HickupHH3 commented 2 years ago

Med severity is justified because, while very unlikely to happen, there could be a loss of assets.

code-423n4 / 2022-06-putty-findings