Putty contest - robee's results

Require with empty message

The following requires are with empty messages. This is very important to add a message for any require. So the user has enough information to know the reason of failure.

Code instances:

    Solidity file: SafeCastLib.sol, In line 33 with Empty Require message.
    Solidity file: SafeCastLib.sol, In line 39 with Empty Require message.
    Solidity file: SafeCastLib.sol, In line 51 with Empty Require message.
    Solidity file: SafeCastLib.sol, In line 57 with Empty Require message.
    Solidity file: SafeCastLib.sol, In line 45 with Empty Require message.
    Solidity file: SafeCastLib.sol, In line 21 with Empty Require message.
    Solidity file: SafeCastLib.sol, In line 9 with Empty Require message.
    Solidity file: SafeCastLib.sol, In line 27 with Empty Require message.
    Solidity file: SafeCastLib.sol, In line 15 with Empty Require message.

Not verified input

external / public functions parameters should be validated to make sure the address is not 0. Otherwise if not given the right input it can mistakenly lead to loss of user funds.

Code instances:

    ERC20.sol.permit spender
    PuttyV2Nft.sol.transferFrom to
    ERC20.sol.transfer to
    ERC4626.sol.redeem receiver
    ERC20.sol.approve spender
    ERC20.sol.transferFrom from
    MultiRolesAuthority.sol.setTargetCustomAuthority target
    ERC4626.sol.withdraw receiver
    MultiRolesAuthority.sol.setUserRole user
    RolesAuthority.sol.setRoleCapability target
    RolesAuthority.sol.setUserRole user
    ERC4626.sol.mint receiver
    ERC4626.sol.deposit receiver
    RolesAuthority.sol.setPublicCapability target
    ERC20.sol.transferFrom to

Solidity compiler versions mismatch

The project is compiled with different versions of solidity, which is not recommended because it can lead to undefined behaviors.

Use safe math for solidity version <8

You should use safe math for solidity version <8 since there is no default over/under flow check it suchversions of solidity.

Code instances:

    The contract console2.sol doesn't use safe math and is of solidity version < 8
    The contract Vm.sol doesn't use safe math and is of solidity version < 8
    The contract console.sol doesn't use safe math and is of solidity version < 8
    The contract Script.sol doesn't use safe math and is of solidity version < 8
    The contract Test.sol doesn't use safe math and is of solidity version < 8

Not verified owner

    owner param should be validated to make sure the owner address is not address(0).
    Otherwise if not given the right input all only owner accessible functions will be unaccessible.
    
    

Code instances:

    Owned.sol.setOwner newOwner
    ERC4626.sol.redeem owner
    ERC4626.sol.withdraw owner
    ERC20.sol.permit owner

Named return issue

Users can mistakenly think that the return value is the named return, but it is actually the actualreturn statement that comes after. To know that the user needs to read the code and is confusing. Furthermore, removing either the actual return or the named return will save gas.

Code instance:

    FixedPointMathLib.sol, mulDivUp

Two Steps Verification before Transferring Ownership

The following contracts have a function that allows them an admin to change it to a different address. If the admin accidentally uses an invalid address for which they do not have the private key, then the system gets locked. It is important to have two steps admin change where the first is announcing a pending new admin and the new address should then claim its ownership. A similar issue was reported in a previous contest and was assigned a severity of medium: code-423n4/2021-06-realitycards-findings#105

Code instance:

    Owned.sol

Check transfer receiver is not 0 to avoid burned money

Transferring tokens to the zero address is usually prohibited to accidentally avoid "burning" tokens by sending them to an unrecoverable zero address.

Code instances:

    https://github.com/code-423n4/2022-06-putty/tree/main/contracts/lib/solmate/src/tokens/ERC20.sol#L204
    https://github.com/code-423n4/2022-06-putty/tree/main/contracts/lib/solmate/src/mixins/ERC4626.sol#L50
    https://github.com/code-423n4/2022-06-putty/tree/main/contracts/lib/solmate/src/mixins/ERC4626.sol#L115

Add a timelock

To give more trust to users: functions that set key/critical variables should be put behind a timelock.

Code instances:

    https://github.com/code-423n4/2022-06-putty/tree/main/contracts/lib/solmate/src/auth/authorities/RolesAuthority.sol#L95
    https://github.com/code-423n4/2022-06-putty/tree/main/contracts/lib/solmate/src/auth/authorities/MultiRolesAuthority.sol#L82
    https://github.com/code-423n4/2022-06-putty/tree/main/contracts/lib/solmate/src/auth/authorities/RolesAuthority.sol#L66

More efficient Struct packing of StdStorage in the contract Test.sol

    The following structs could change the order of their stored elements to decrease memory uses.
    and number of occupied slots. Therefore will save gas at every store and load from memory.
    

In Test.sol, StdStorage is optimized to: 6 slots from: 7 slots. The new order of types (you choose the actual variables): 1. mapping 2. mapping 3. bytes32[] 4. uint256 5. bytes32 6. address 7. bytes4

Use unchecked to save gas for certain additive calculations that cannot overflow

You can use unchecked in the following calculations since there is no risk to overflow:

Code instance:

    Test.sol (L#23) - vm.warp(block.timestamp + time);

Consider inline the following functions to save gas

You can inline the following functions instead of writing a specific function to save gas.
(see https://github.com/code-423n4/2021-11-nested-findings/issues/167 for a similar issue.)

Code instances

    Bytes32AddressLib.sol, fillLast12Bytes, { return bytes32(bytes20(addressValue)); }
    Bytes32AddressLib.sol, fromLast20Bytes, { return address(uint160(uint256(bytesValue))); }
    FixedPointMathLib.sol, divWadUp, { return mulDivUp(x, WAD, y); // Equivalent to (x * WAD) / y rounded up. }
    FixedPointMathLib.sol, mulWadUp, { return mulDivUp(x, y, WAD); // Equivalent to (x * y) / WAD rounded up. }
    FixedPointMathLib.sol, divWadDown, { return mulDivDown(x, WAD, y); // Equivalent to (x * WAD) / y rounded down. }
    FixedPointMathLib.sol, mulWadDown, { return mulDivDown(x, y, WAD); // Equivalent to (x * y) / WAD rounded down. }

Inline one time use functions

The following functions are used exactly once. Therefore you can inline them and save gas and improve code clearness.

Code instances:

    FixedPointMathLib.sol, sqrt
    CREATE3.sol, getDeployed

Upgrade pragma to at least 0.8.4

Using newer compiler versions and the optimizer gives gas optimizations and additional safety checks are available for free.

The advantages of versions 0.8.* over <0.8.0 are:

    1. Safemath by default from 0.8.0 (can be more gas efficient than library based safemath.)
    2. Low level inliner : from 0.8.2, leads to cheaper runtime gas. Especially relevant when the contract has small functions. For example, OpenZeppelin libraries typically have a lot of small helper functions and if they are not inlined, they cost an additional 20 to 40 gas because of 2 extra jump instructions and additional stack operations needed for function calls.
    3. Optimizer improvements in packed structs: Before 0.8.3, storing packed structs, in some cases used an additional storage read operation. After EIP-2929, if the slot was already cold, this means unnecessary stack operations and extra deploy time costs. However, if the slot was already warm, this means additional cost of 100 gas alongside the same unnecessary stack operations and extra deploy time costs.
    4. Custom errors from 0.8.4, leads to cheaper deploy time cost and run time cost. Note: the run time cost is only relevant when the revert condition is met. In short, replace revert strings by custom errors.

Code instances:

    Script.sol
    console2.sol
    Vm.sol
    Test.sol
    console.sol