Putty contest - catchup's results

Lines of code

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

Vulnerability details

Impact

Admin can update the fee by calling setFee() function. Such changes may effect user decisions, hence they should have some safeguards such as timelocks. This way users can have the necessary time to update their decisions.

Proof of Concept

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

Tools Used

Manual review

Recommended Mitigation Steps

A timelock can be added for such critical changes which may effect user decisions.

Lines of code

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

Vulnerability details

Impact

cancel() function does not check if the order has already been filled. A maker can cancel the order even if it was filled, thinking he cancelled the order when actually he did not. If the maker is short, long counterparty can exercise the position at a later time and short maker would find himself in an unexpected situation.

Proof of Concept

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

Tools Used

Manual review

Recommended Mitigation Steps

I suggest to check the value of positionExpirations of the order. If it is non-zero and not expired, that means position is filled and not expired yet. In such a case, revert the cancellation operation informing the user that he can't cancel a filled order.

for loops index increments can be made unchecked

For loop index increments can be made unchecked for solidity versions > 0.8.0. There is no risk of overflowing the index of the for loops.

Lines of code

10 instances: https://github.com/code-423n4/2022-06-putty/blob/main/contracts/src/PuttyV2.sol#L556 https://github.com/code-423n4/2022-06-putty/blob/main/contracts/src/PuttyV2.sol#L594 https://github.com/code-423n4/2022-06-putty/blob/main/contracts/src/PuttyV2.sol#L611 https://github.com/code-423n4/2022-06-putty/blob/main/contracts/src/PuttyV2.sol#L627 https://github.com/code-423n4/2022-06-putty/blob/main/contracts/src/PuttyV2.sol#L637 https://github.com/code-423n4/2022-06-putty/blob/main/contracts/src/PuttyV2.sol#L647 https://github.com/code-423n4/2022-06-putty/blob/main/contracts/src/PuttyV2.sol#L658 https://github.com/code-423n4/2022-06-putty/blob/main/contracts/src/PuttyV2.sol#L670 https://github.com/code-423n4/2022-06-putty/blob/main/contracts/src/PuttyV2.sol#L728 https://github.com/code-423n4/2022-06-putty/blob/main/contracts/src/PuttyV2.sol#L742

Recommended Mitigation Steps

I suggest to change the original code from this

        for (uint256 i = 0; i < whitelist.length; i++) {
            if (target == whitelist[i]) return true;
        }

to this:

        for (uint256 i; i < whitelist.length; ) {
            if (target == whitelist[i]) return true;
            unchecked { ++i; }
        }

Prefix increment/decrements are cheaper than postfix

Prefix increment/decrements (++i) is about 5 gas cheaper than postfix increment/decrements (i++). The reason of this is i++ stores the value of i initially and than increments, meaning an additional memory location is required for the operation.

Lines of code

10 instances: https://github.com/code-423n4/2022-06-putty/blob/main/contracts/src/PuttyV2.sol#L556 https://github.com/code-423n4/2022-06-putty/blob/main/contracts/src/PuttyV2.sol#L594 https://github.com/code-423n4/2022-06-putty/blob/main/contracts/src/PuttyV2.sol#L611 https://github.com/code-423n4/2022-06-putty/blob/main/contracts/src/PuttyV2.sol#L627 https://github.com/code-423n4/2022-06-putty/blob/main/contracts/src/PuttyV2.sol#L637 https://github.com/code-423n4/2022-06-putty/blob/main/contracts/src/PuttyV2.sol#L647 https://github.com/code-423n4/2022-06-putty/blob/main/contracts/src/PuttyV2.sol#L658 https://github.com/code-423n4/2022-06-putty/blob/main/contracts/src/PuttyV2.sol#L670 https://github.com/code-423n4/2022-06-putty/blob/main/contracts/src/PuttyV2.sol#L728 https://github.com/code-423n4/2022-06-putty/blob/main/contracts/src/PuttyV2.sol#L742

Redundant initialisation with default value

There is no need the initialise uint to 0, bool to false, address to address(0), since they default to these values. It uses extra 8 gas to init stack variables.

Lines of code

11 instances: https://github.com/code-423n4/2022-06-putty/blob/main/contracts/src/PuttyV2.sol#L497 https://github.com/code-423n4/2022-06-putty/blob/main/contracts/src/PuttyV2.sol#L556 https://github.com/code-423n4/2022-06-putty/blob/main/contracts/src/PuttyV2.sol#L594 https://github.com/code-423n4/2022-06-putty/blob/main/contracts/src/PuttyV2.sol#L611 https://github.com/code-423n4/2022-06-putty/blob/main/contracts/src/PuttyV2.sol#L627 https://github.com/code-423n4/2022-06-putty/blob/main/contracts/src/PuttyV2.sol#L637 https://github.com/code-423n4/2022-06-putty/blob/main/contracts/src/PuttyV2.sol#L647 https://github.com/code-423n4/2022-06-putty/blob/main/contracts/src/PuttyV2.sol#L658 https://github.com/code-423n4/2022-06-putty/blob/main/contracts/src/PuttyV2.sol#L670 https://github.com/code-423n4/2022-06-putty/blob/main/contracts/src/PuttyV2.sol#L728 https://github.com/code-423n4/2022-06-putty/blob/main/contracts/src/PuttyV2.sol#L742

fee can be cached in withdraw() function

fee is accessed twice in withdraw function; on lines L498 and L499. It can be cached and used from stack to save an SLOAD, which would save ~97 gas.

Lines of code

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

Constants can be made private

Using public visibility for constants would cause getter function creations for them which would cause unnecessary gas usage.

Lines of code

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