Putty contest - sashik_eth's results

Lines of code

https://github.com/code-423n4/2022-06-putty/blob/main/contracts/src/PuttyV2.sol#L466-L520 https://github.com/code-423n4/2022-06-putty/blob/main/contracts/src/PuttyV2.sol#L636-L661

Vulnerability details

Impact

withdraw() function calls internal functions that tranfers assets - _transferERC20sOut, _transferERC721sOut, _transferFloorsOut. They could only run through all transferring assets in one cycle. If one of transferring assets has malicious code that blocks all transfers, except those allowed by an attacker, then an attacker could demand a ransom from the victim for allowing the withdrawal of blocked assets.

    /**
        @notice Transfers an array of erc20 tokens to the msg.sender.
        @param assets The erc20 tokens and amounts to send.
     */
    function _transferERC20sOut(ERC20Asset[] memory assets) internal {
        for (uint256 i = 0; i < assets.length; i++) {
            ERC20(assets[i].token).safeTransfer(msg.sender, assets[i].tokenAmount);
        }
    }

    /**
        @notice Transfers an array of erc721 tokens to the msg.sender.
        @param assets The erc721 tokens and token ids to send.
     */
    function _transferERC721sOut(ERC721Asset[] memory assets) internal {
        for (uint256 i = 0; i < assets.length; i++) {
            ERC721(assets[i].token).safeTransferFrom(address(this), msg.sender, assets[i].tokenId);
        }
    }

    /**
        @notice Transfers an array of erc721 floor tokens to the msg.sender.
        @param floorTokens The contract addresses for each floor token.
        @param floorTokenIds The token id of each floor token.
     */
    function _transferFloorsOut(address[] memory floorTokens, uint256[] memory floorTokenIds) internal {
        for (uint256 i = 0; i < floorTokens.length; i++) {
            ERC721(floorTokens[i]).safeTransferFrom(address(this), msg.sender, floorTokenIds[i]);
        }
    }

Proof of Concept

Example of attack:

• attacker creates order with a batch of NFTs: one Bored Ape and one MaliciousNFT(only attacker could allow transfer of this NFT)
• Alice takes the attacker's order since it has liquid NFT (Bored Ape) and good conditions (premium size, strike price, etc)
• attacker exercise order, receiving Alice strike
• Alice trying to withdraw her assets, but transaction reverts since transferring of MaliciousNFT was blocked by attacker
• attacker contact Alice and demand a ransom for allowing withdrawal of blocked assets.

Recommended Mitigation Steps

Add functionality that allows withdrawing specific assets apart from a batch.

Judge has assessed an item in Issue #358 as Medium risk. The relevant finding follows:

Lines of code

https://github.com/code-423n4/2022-06-putty/blob/main/contracts/src/PuttyV2.sol#L497-L501

Vulnerability details

Impact

In withdraw() function if order is exercised call or not exercised long and fee > 0, contract takes some feeAmount to owner address, charged in baseAsset. But in case when feeAmount would equal zero (due to low strike price and low fee) and baseAsset contract doesn’t allow for zero amount transfers, withdraw function would be blocked.

Proof of Concept

If fee is 1 and order.strike is 999 wei, feeAmount would be counted as 0 and transfer to owner() address will always revert. Setting fee to 0 value can be impossible or quite hard if owner() address would be controlled by Dao.

            ...
            uint256 feeAmount = 0;
            if (fee > 0) {
                feeAmount = (order.strike * fee) / 1000; // @audit note magic number
                ERC20(order.baseAsset).safeTransfer(owner(), feeAmount); // @audit low malicious owner can broke
            }
            ...

Recommended Mitigation Steps

Add check for feeAmount > 0:

            uint256 feeAmount = 0;
            if (fee > 0) {
                feeAmount = (order.strike * fee) / 1000;
                if (feeAmount > 0) {
                    ERC20(order.baseAsset).safeTransfer(owner(), feeAmount);
                }
            }

L01 - Malicious owner() could block withdraw

Transfer of feeAmount in withdraw() function could be reverted by malicious contract on owner address, which would lead to withdrawing block:

            if (fee > 0) {
                feeAmount = (order.strike * fee) / 1000;
                ERC20(order.baseAsset).safeTransfer(owner(), feeAmount);
            }

Recommendation: use transfer function for transferring owners fee.

L02 - withdraw() may run out of gas if contracts code of underliying assets would changed in future

Internal functions that withdraw assets _transferERC20sOut, _transferERC721sOut, _transferFloorsOut could only run through all transferring assets in one cycle. If some contracts of transferring assets were updated in future and their transfer would cost more gas, this could lead to inability to withdraw user assets.

Recommendation: Add functionality that allows withdrawing specific assets apart from a batch.

L03 - msg.value could be locked

fillOrder() and exercise() functions should check if msg.value == 0 in case if order.baseAsset != weth. Otherwise user's ether could stuck on contract.

N01 - Missing two-step changing ownership procedure

If the wrong address was used to transfer ownership (for which the private key is unknown) by accident, then it permanently prevents all onlyOwner() functions from being used, including changing various critical parameters.

N02 - Not used import

contracts/src/PuttyV2Nft.sol:5  import "openzeppelin/utils/Strings.sol"; 

https://github.com/code-423n4/2022-06-putty/blob/main/contracts/src/PuttyV2Nft.sol#L5

N03 - Missing NatSpec

All functions in PuttyV2Nft.sol

G01 - More efficient if/else statement

Next lines L342-L379 could be updated to equivalent equivalent code with fewer checks:

        if(order.isCall) {
            if (order.isLong) {
                _transferERC20sIn(order.erc20Assets, msg.sender);
                _transferERC721sIn(order.erc721Assets, msg.sender);
                _transferFloorsIn(order.floorTokens, floorAssetTokenIds, msg.sender);
                return positionId;
            } else {
                _transferERC20sIn(order.erc20Assets, order.maker);
                _transferERC721sIn(order.erc721Assets, order.maker);
                return positionId;
            }
        } else {
            if (order.isLong) {
                // handle the case where the taker uses native ETH instead of WETH to deposit the strike
                if (weth == order.baseAsset && msg.value > 0) {
                    // check enough ETH was sent to cover the strike
                    require(msg.value == order.strike, "Incorrect ETH amount sent"); 

                    // convert ETH to WETH
                    // we convert the strike ETH to WETH so that the logic in exercise() works
                    // - because exercise() assumes an ERC20 interface on the base asset.
                    IWETH(weth).deposit{value: msg.value}();
                } else {
                    ERC20(order.baseAsset).safeTransferFrom(msg.sender, address(this), order.strike);
                }

                return positionId;
            } else { 
                ERC20(order.baseAsset).safeTransferFrom(order.maker, address(this), order.strike);
                return positionId;
            }
        }

G02 - Use unchecked for calculations that cannot overflow/underflow

Next line could be unchecked, because due to L499 feeAmount always will be less than order.strike (since fee < 30 due to L241 ):

contracts/src/PuttyV2.sol:503   ERC20(order.baseAsset).safeTransfer(msg.sender, order.strike - feeAmount);

Next line could be unchecked since order.duration < 10 000 days(L287):

contracts/src/PuttyV2.sol:316   positionExpirations[order.isLong ? uint256(orderHash) : positionId] = block.timestamp + order.duration; 

Counter i++ in for loop could be unchecked when it cann't overflow:

contracts/src/PuttyV2.sol:556   for (uint256 i = 0; i < orders.length; i++) { 

contracts/src/PuttyV2.sol:594   for (uint256 i = 0; i < assets.length; i++) { 

contracts/src/PuttyV2.sol:611   for (uint256 i = 0; i < assets.length; i++) {

contracts/src/PuttyV2.sol:627   for (uint256 i = 0; i < floorTokens.length; i++) {

contracts/src/PuttyV2.sol:637   for (uint256 i = 0; i < assets.length; i++) {

contracts/src/PuttyV2.sol:647   for (uint256 i = 0; i < assets.length; i++) {

contracts/src/PuttyV2.sol:658   for (uint256 i = 0; i < floorTokens.length; i++) {

contracts/src/PuttyV2.sol:670   for (uint256 i = 0; i < whitelist.length; i++) {

contracts/src/PuttyV2.sol:728   for (uint256 i = 0; i < arr.length; i++) {

contracts/src/PuttyV2.sol:742   for (uint256 i = 0; i < arr.length; i++) {

G03 - Prefix increment ++i in for loop is cheaper than postfix increment i++

In next loops using ++i instead of i++ could save gas:

contracts/src/PuttyV2.sol:556   for (uint256 i = 0; i < orders.length; i++) { 

contracts/src/PuttyV2.sol:594   for (uint256 i = 0; i < assets.length; i++) { 

contracts/src/PuttyV2.sol:611   for (uint256 i = 0; i < assets.length; i++) {

contracts/src/PuttyV2.sol:627   for (uint256 i = 0; i < floorTokens.length; i++) {

contracts/src/PuttyV2.sol:637   for (uint256 i = 0; i < assets.length; i++) {

contracts/src/PuttyV2.sol:647   for (uint256 i = 0; i < assets.length; i++) {

contracts/src/PuttyV2.sol:658   for (uint256 i = 0; i < floorTokens.length; i++) {

contracts/src/PuttyV2.sol:670   for (uint256 i = 0; i < whitelist.length; i++) {

contracts/src/PuttyV2.sol:728   for (uint256 i = 0; i < arr.length; i++) {

contracts/src/PuttyV2.sol:742   for (uint256 i = 0; i < arr.length; i++) {

G04 - Caching array length before for loops

contracts/src/PuttyV2.sol:556   for (uint256 i = 0; i < orders.length; i++) { 

contracts/src/PuttyV2.sol:594   for (uint256 i = 0; i < assets.length; i++) { 

contracts/src/PuttyV2.sol:611   for (uint256 i = 0; i < assets.length; i++) {

contracts/src/PuttyV2.sol:627   for (uint256 i = 0; i < floorTokens.length; i++) {

contracts/src/PuttyV2.sol:637   for (uint256 i = 0; i < assets.length; i++) {

contracts/src/PuttyV2.sol:647   for (uint256 i = 0; i < assets.length; i++) {

contracts/src/PuttyV2.sol:658   for (uint256 i = 0; i < floorTokens.length; i++) {

contracts/src/PuttyV2.sol:670   for (uint256 i = 0; i < whitelist.length; i++) {

contracts/src/PuttyV2.sol:728   for (uint256 i = 0; i < arr.length; i++) {

contracts/src/PuttyV2.sol:742   for (uint256 i = 0; i < arr.length; i++) {

Caching the array length before for loop could save gas here:

G05 - Using >0 costs more gas than !=0 with uint in require statement

contracts/src/PuttyV2.sol:293   require(order.baseAsset.code.length > 0, "baseAsset is not contract"); 

contracts/src/PuttyV2.sol:598   require(token.code.length > 0, "ERC20: Token is not contract"); 

contracts/src/PuttyV2.sol:599   require(tokenAmount > 0, "ERC20: Amount too small"); 

G06 - Using custom error instead of require() string

Custom errors are available from solidity version 0.8.4. They cost cheaper than require/revert strings.

contracts/src/PuttyV2.sol:214   require(_weth != address(0), "Unset weth address"); 

contracts/src/PuttyV2.sol:241   require(_fee < 30, "fee must be less than 3%"); 

contracts/src/PuttyV2.sol:278   require(SignatureChecker.isValidSignatureNow(order.maker, orderHash, signature), "Invalid signature"); 

contracts/src/PuttyV2.sol:281   require(!cancelledOrders[orderHash], "Order has been cancelled"); 

contracts/src/PuttyV2.sol:284   require(order.whitelist.length == 0 || isWhitelisted(order.whitelist, msg.sender), "Not whitelisted"); 

contracts/src/PuttyV2.sol:287   require(order.duration < 10_000 days, "Duration too long"); 

contracts/src/PuttyV2.sol:290   require(block.timestamp < order.expiration, "Order has expired"); 

contracts/src/PuttyV2.sol:293   require(order.baseAsset.code.length > 0, "baseAsset is not contract"); 

contracts/src/PuttyV2.sol:297   ? require(floorAssetTokenIds.length == order.floorTokens.length, "Wrong amount of floor tokenIds") 

contracts/src/PuttyV2.sol:298   : require(floorAssetTokenIds.length == 0, "Invalid floor tokens length"); 

contracts/src/PuttyV2.sol:353   require(msg.value == order.strike, "Incorrect ETH amount sent"); 

contracts/src/PuttyV2.sol:395   require(ownerOf(uint256(orderHash)) == msg.sender, "Not owner"); 

contracts/src/PuttyV2.sol:398   require(order.isLong, "Can only exercise long positions");  

contracts/src/PuttyV2.sol:401   require(block.timestamp < positionExpirations[uint256(orderHash)], "Position has expired"); 

contracts/src/PuttyV2.sol:405   ? require(floorAssetTokenIds.length == order.floorTokens.length, "Wrong amount of floor tokenIds") 

contracts/src/PuttyV2.sol:406   : require(floorAssetTokenIds.length == 0, "Invalid floor tokenIds length"); 

contracts/src/PuttyV2.sol:429   require(msg.value == order.strike, "Incorrect ETH amount sent"); 

contracts/src/PuttyV2.sol:470   require(!order.isLong, "Must be short position"); 

contracts/src/PuttyV2.sol:475   require(ownerOf(uint256(orderHash)) == msg.sender, "Not owner"); 

contracts/src/PuttyV2.sol:481   require(block.timestamp > positionExpirations[longPositionId] || isExercised, "Must be exercised or expired"); 

contracts/src/PuttyV2.sol:527   require(msg.sender == order.maker, "Not your order"); 

contracts/src/PuttyV2.sol:551   require(orders.length == signatures.length, "Length mismatch in input"); 

contracts/src/PuttyV2.sol:552   require(signatures.length == floorAssetTokenIds.length, "Length mismatch in input"); 

contracts/src/PuttyV2.sol:598   require(token.code.length > 0, "ERC20: Token is not contract"); 

contracts/src/PuttyV2.sol:599   require(tokenAmount > 0, "ERC20: Amount too small"); 

contracts/src/PuttyV2.sol:765   require(_ownerOf[id] != address(0), "URI query for NOT_MINTED token"); 

contracts/src/PuttyV2Nft.sol:12 require(to != address(0), "INVALID_RECIPIENT"); 

contracts/src/PuttyV2Nft.sol:13 require(_ownerOf[id] == address(0), "ALREADY_MINTED"); 

contracts/src/PuttyV2Nft.sol:26 require(from == _ownerOf[id], "WRONG_FROM"); 

contracts/src/PuttyV2Nft.sol:27 require(to != address(0), "INVALID_RECIPIENT"); 

contracts/src/PuttyV2Nft.sol:30 "NOT_AUTHORIZED"

contracts/src/PuttyV2Nft.sol:41 require(owner != address(0), "ZERO_ADDRESS");