Putty contest - simon135's results

on deployment or a new orderhash this below will equal 0

PuttyV2.sol:312:            positionFloorAssetTokenIds[uint256(orderHash)] = floorAssetTokenIds;

causing no tokenIds to be proccessed

you can have a token where dosnt implement the safetransferfrom function properly and it will revert

or get your nft stuck in the contract

PuttyV2.sol:648:            ERC721(assets[i].token).safeTransferFrom(address(this), msg.sender, assets[i].tokenId);
PuttyV2.sol:659:            ERC721(floorTokens[i]).safeTransferFrom(address(this), msg.sender, floorTokenIds[i]);

a non impact reentracy with baseasset token where you can change the implemnation

it is little harder than if it was an interface but you can change it becauase the attacker has supply of giving there own token to implement on like baseAsset= attacker token ERC20(order.baseAsset).safeTransferFrom(order.maker, msg.sender, order.premium);

order.maker if maker dosnt give allowance to msg.sender the function will revert

ERC20(order.baseAsset).safeTransferFrom(msg.sender, address(this), order.strike);

PuttyV2.sol:324:            ERC20(order.baseAsset).safeTransferFrom(order.maker, msg.sender, order.premium);
PuttyV2.sol:338:                ERC20(order.baseAsset).safeTransferFrom(msg.sender, order.maker, order.premium);
PuttyV2.sol:344:            ERC20(order.baseAsset).safeTransferFrom(order.maker, address(this), order.strike);
PuttyV2.sol:360:                ERC20(order.baseAsset).safeTransferFrom(msg.sender, address(this), order.strike);
PuttyV2.sol:436:                ERC20(order.baseAsset).safeTransferFrom(msg.sender, address(this), order.strike);
PuttyV2.sol:451:            ERC20(order.baseAsset).safeTransfer(msg.sender, order.strike);
PuttyV2.sol:500:                ERC20(order.baseAsset).safeTransfer(owner(), feeAmount);
PuttyV2.sol:503:            ERC20(order.baseAsset).safeTransfer(msg.sender, order.strike - feeAmount);
PuttyV2.sol:601:            ERC20(token).safeTransferFrom(from, address(this), tokenAmount);
PuttyV2.sol:612:            ERC721(assets[i].token).safeTransferFrom(from, address(this), assets[i].tokenId);
PuttyV2.sol:628:            ERC721(floorTokens[i]).safeTransferFrom(from, address(this), floorTokenIds[i]);
PuttyV2.sol:638:            ERC20(assets[i].token).safeTransfer(msg.sender, assets[i].tokenAmount);
PuttyV2.sol:648:            ERC721(assets[i].token).safeTransferFrom(address(this), msg.sender, assets[i].tokenId);
PuttyV2.sol:659:            ERC721(floorTokens[i]).safeTransferFrom(address(this), msg.sender, floorTokenIds[i]);

if order.baseAsset is not weth token but you byaccdient supply msg.value > 0 then your eth is lost in the contract for ever.

1. attacker

PuttyV2.sol:327:           
if (weth == order.baseAsset && msg.value > 0) {
PuttyV2.sol:351
PuttyV2.sol:427

have a fallback function for dealing with this.

you can possibly drain other users tokens if order.maker is victim and has allowance with this contract

ERC20(order.baseAsset).safeTransferFrom(order.maker, address(this), order.strike);

baseAsset is lets say is usdc, then order.maker is victim that has an allowance with address(this) then it transfers strike price to this address allowing an attacker to manupliate a call and loss funds for users

if some one can make their own array of token and amount then you can have victim that approves (adress(this)) and loose funds

PuttyV2.sol:595:            address token = assets[i].token;
PuttyV2.sol:596:            uint256 tokenAmount = assets[i].tokenAmount;

 address token = assets[i].token;
            uint256 tokenAmount = assets[i].tokenAmount;
 
            require(token.code.length > 0, "ERC20: Token is not contract");
            require(tokenAmount > 0, "ERC20: Amount too small");
 
            ERC20(token).safeTransferFrom(from, address(this), tokenAmount);
 
 

bob approves 5 weth to this(address) an attacker makes an array with array[ weth,5] and victim is bob so an attacker has to front run fillorder call and have maker as bob and bob will loose tokens

you can reentry with token if the token is an attacker token then it wont transfer721in in function fillorder()

require (msg.value== order.strike) is very hard since of strick and it can revert easily by msg.value being a little less then order.strike or something happens to value or if its not exactly equal it will fail which is to strict in my opnion

mitigation

require(order.strike => msg.value);

dont use safetransfer it can revert do to if a token is transfering 0 first

some tokens will revert if you transfer 0 first ERC20(order.baseAsset).safeTransfer(msg.sender, order.strike);

puttyv2.sol 500:             ERC20(order.baseAsset).safeTransfer(owner(), feeAmount);
PuttyV2.sol:503:            ERC20(order.baseAsset).safeTransfer(msg.sender, order.strike - feeAmount);
# remove the emit in the middle function and its just best practice
https://github.com/code-423n4/2022-06-putty/blob/1ddbec4a5242e0160da832cb46b2b3cdbb49a8af/contracts/src/PuttyV2.sol#L490

small percision issue with feeamount

  feeAmount = (order.strike * fee) / 1000;
                ERC20(order.baseAsset).safeTransfer(owner(), feeAmount);

ex: fee=10 , if order.strike is less 100 then attacker is paying no fees and if you can manuplite baseasset to be a small decimal token then it can be more of an issue ex:gemeni usc =2 decimals and not pay fees up to $1 instead of 0.5 dollar making to more worth it, also if its on another chain like polygon where fees are lost less making it more appezing

balance of function you can remove the require check and the parmeter and just return the number

PuttyV2Nft.sol:40

    function balanceOf(address owner) public pure override returns (uint256) {
        require(owner != address(0), "ZERO_ADDRESS");
 
        return type(uint256).max;
 

no emit of old variable (fee) its just best practice

instead of : emit(fee) use of : emit(oldfee,fee)

PuttyV2.sol:245:        emit NewFee(_fee);

1. when using keccak hash into a variable make it immutable

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

3. make the constants private or internal to save gas when there only used in this contract

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

dont use abi.encode or abi.encodepacked, instead use string.concat

to save gas

90:        keccak256(abi.encodePacked("ERC721Asset(address token,uint256 tokenId)"));
96:        keccak256(abi.encodePacked("ERC20Asset(address token,uint256 tokenAmount)"));
103:            abi.encodePacked(
685:        Order memory oppositeOrder = abi.decode(abi.encode(order), (Order));
701:            abi.encode(
712:                keccak256(abi.encodePacked(order.whitelist)),
713:                keccak256(abi.encodePacked(order.floorTokens)),
729:            encoded = abi.encodePacked(
731:                keccak256(abi.encode(ERC20ASSET_TYPE_HASH, arr[i].token, arr[i].tokenAmount))
743:            encoded = abi.encodePacked(
745:                keccak256(abi.encode(ERC721ASSET_TYPE_HASH, arr[i].token, arr[i].tokenId))

++i costs less gas compared to i++ or i += 1

++i costs less gas compared to i++ or i += 1 for unsigned integer, as pre-increment is cheaper (about 5 gas per iteration). This statement is true even with the optimizer enabled. i++ increments i and returns the initial value of i. Which means: uint i = 1; i++; // == 1 but i == 2 But ++i returns the actual incremented value: uint i = 1; ++i; // == 2 and i == 2 too, so no need for a temporary variable In the first case, the compiler has to create a temporary variable (when used) for returning 1 instead of 2 uint256 i = 0; i < depositedTokens.values.length; i++)

556:        for (uint256 i = 0; i < orders.length; i++) {
594:        for (uint256 i = 0; i < assets.length; i++) {
611:        for (uint256 i = 0; i < assets.length; i++) {
627:        for (uint256 i = 0; i < floorTokens.length; i++) {
637:        for (uint256 i = 0; i < assets.length; i++) {
647:        for (uint256 i = 0; i < assets.length; i++) {
658:        for (uint256 i = 0; i < floorTokens.length; i++) {
670:        for (uint256 i = 0; i < whitelist.length; i++) {
728:        for (uint256 i = 0; i < arr.length; i++) {
742:        for (uint256 i = 0; i < arr.length; i++) {

Use Custom Errors instead of Revert Strings to save Gas

Custom errors from Solidity 0.8.4 are cheaper than revert strings (cheaper deployment cost and runtime cost when the revert condition is met) Source Custom Errors in Solidity: Starting from Solidity v0.8.4, there is a convenient and gas-efficient way to explain to users why an operation failed through the use of custom errors. Until now, you could already use strings to give more information about failures (e.g., revert("Insufficient funds.");), but they are rather expensive, especially when it comes to deploy cost, and it is difficult to use dynamic information in them. Custom errors are defined using the error statement, which can be used inside and outside of contracts (including interfaces and libraries). instances include:

214:        require(_weth != address(0), "Unset weth address");
241:        require(_fee < 30, "fee must be less than 3%");
278:        require(SignatureChecker.isValidSignatureNow(order.maker, orderHash, signature), "Invalid signature");
281:        require(!cancelledOrders[orderHash], "Order has been cancelled");
284:        require(order.whitelist.length == 0 || isWhitelisted(order.whitelist, msg.sender), "Not whitelisted");
287:        require(order.duration < 10_000 days, "Duration too long");
290:        require(block.timestamp < order.expiration, "Order has expired");
293:        require(order.baseAsset.code.length > 0, "baseAsset is not contract");
297:            ? require(floorAssetTokenIds.length == order.floorTokens.length, "Wrong amount of floor tokenIds")
298:            : require(floorAssetTokenIds.length == 0, "Invalid floor tokens length");
329:                require(msg.value == order.premium, "Incorrect ETH amount sent");
353:                require(msg.value == order.strike, "Incorrect ETH amount sent");
395:        require(ownerOf(uint256(orderHash)) == msg.sender, "Not owner");
398:        require(order.isLong, "Can only exercise long positions");
401:        require(block.timestamp < positionExpirations[uint256(orderHash)], "Position has expired");
405:            ? require(floorAssetTokenIds.length == order.floorTokens.length, "Wrong amount of floor tokenIds")
406:            : require(floorAssetTokenIds.length == 0, "Invalid floor tokenIds length");
429:                require(msg.value == order.strike, "Incorrect ETH amount sent");
470:        require(!order.isLong, "Must be short position");
475:        require(ownerOf(uint256(orderHash)) == msg.sender, "Not owner");
481:        require(block.timestamp > positionExpirations[longPositionId] || isExercised, "Must be exercised or expired");
527:        require(msg.sender == order.maker, "Not your order");
551:        require(orders.length == signatures.length, "Length mismatch in input");
552:        require(signatures.length == floorAssetTokenIds.length, "Length mismatch in input");
598:            require(token.code.length > 0, "ERC20: Token is not contract");
599:            require(tokenAmount > 0, "ERC20: Amount too small");
765:        require(_ownerOf[id] != address(0), "URI query for NOT_MINTED token");

make variables uninitlized to save gas

beause a variable of 0 is uninlized and saves gas instead of initizling the variable and not saving gas atall and it being the same value.

556:        for (uint256 i = 0; i < orders.length; i++) {
594:        for (uint256 i = 0; i < assets.length; i++) {
611:        for (uint256 i = 0; i < assets.length; i++) {
627:        for (uint256 i = 0; i < floorTokens.length; i++) {
637:        for (uint256 i = 0; i < assets.length; i++) {
647:        for (uint256 i = 0; i < assets.length; i++) {
658:        for (uint256 i = 0; i < floorTokens.length; i++) {
670:        for (uint256 i = 0; i < whitelist.length; i++) {
728:        for (uint256 i = 0; i < arr.length; i++) {
742:        for (uint256 i = 0; i < arr.length; i++) {

dont use abi.encode instead use abi.encodepacked to save gas

731:                keccak256(abi.encode(ERC20ASSET_TYPE_HASH, arr[i].token, arr[i].tokenAmount))
743:            encoded = abi.encodePacked(
745:                keccak256(abi.encode(ERC721ASSET_TYPE_HASH, arr[i].token, arr[i].tokenId))
 

make require statement != 0 instead of > 0 with a uint

598:            require(token.code.length > 0, "ERC20: Token is not contract");
599:            require(tokenAmount > 0, "ERC20: Amount too small");
293:        require(order.baseAsset.code.length > 0, "baseAsset is not contract");

instead of address(0) write it out, it saves gas

use : 0x00000000000000000000000 instead of of : address(0)

765:        require(_ownerOf[id] != address(0), "URI query for NOT_MINTED token");
214:        require(_weth != address(0), "Unset weth address");

Using bools for storage incurs overhead

// Booleans are more expensive than uint256 or any type that takes up a full // word because each write operation emits an extra SLOAD to first read the // slot's contents, replace the bits taken up by the boolean, and then write // back. This is the compiler's defense against contract upgrades and // pointer aliasing, and it cannot be disabled. https://github.com/code-423n4/2022-06-putty/blob/1ddbec4a5242e0160da832cb46b2b3cdbb49a8af/contracts/src/PuttyV2.sol#L143 https://github.com/code-423n4/2022-06-putty/blob/1ddbec4a5242e0160da832cb46b2b3cdbb49a8af/contracts/src/PuttyV2.sol#L153

make memory array, instead use calldata to save gas

https://github.com/code-423n4/2022-06-putty/blob/1ddbec4a5242e0160da832cb46b2b3cdbb49a8af/contracts/src/PuttyV2.sol#L271

Unchecking arithmetics operations that can’t underflow/overflow

Solidity version 0.8+ comes with implicit overflow and underflow checks on unsigned integers. When an overflow or an underflow isn’t possible (as an example, when a comparison is made before the arithmetic operation), some gas can be saved by using an unchecked block: Checked or Unchecked Arithmetic. I suggest wrapping with an unchecked block: Same thing with second unchecked because total can't overflow amount cant overflow

556:        for (uint256 i = 0; i < orders.length; i++) {
594:        for (uint256 i = 0; i < assets.length; i++) {
611:        for (uint256 i = 0; i < assets.length; i++) {
627:        for (uint256 i = 0; i < floorTokens.length; i++) {
637:        for (uint256 i = 0; i < assets.length; i++) {
647:        for (uint256 i = 0; i < assets.length; i++) {
658:        for (uint256 i = 0; i < floorTokens.length; i++) {
670:        for (uint256 i = 0; i < whitelist.length; i++) {
728:        for (uint256 i = 0; i < arr.length; i++) {
742:        for (uint256 i = 0; i < arr.length; i++) {

cache arrays into a variable to save gas instead of geting it length put in to variable

ex: uint line=arr.lenght;

556:        for (uint256 i = 0; i < orders.length; i++) {
594:        for (uint256 i = 0; i < assets.length; i++) {
611:        for (uint256 i = 0; i < assets.length; i++) {
627:        for (uint256 i = 0; i < floorTokens.length; i++) {
637:        for (uint256 i = 0; i < assets.length; i++) {
647:        for (uint256 i = 0; i < assets.length; i++) {
658:        for (uint256 i = 0; i < floorTokens.length; i++) {
670:        for (uint256 i = 0; i < whitelist.length; i++) {
728:        for (uint256 i = 0; i < arr.length; i++) {
742:        for (uint256 i = 0; i < arr.length; i++) {