Phuture Finance contest - Tadashi's results

Inconsistent usage of uint256 and uint

Summary: FullMath.sol, FixedPoint112.sol and vToken.sol (L222) uses uint256 while all other contracts use uint.

Details: To favor explicitness and readability, consider replacing all instances of uint to uint256.

Mitigation: Change uint to uint256 in all contracts.

Impact: Informational/Low risk

Inconsistent solidity pragma

Summary: FullMath.sol has different solidity compiler ranges referenced than the other contracts.

Details: FullMath.sol uses pragma solidity >=0.8.4 <0.9.0; while the other contracts in scope uses pragma solidity >=0.8.7;. This range difference leads to potential security flaws between deployed contracts depending on the compiler version chosen for any particular file. It also greatly increases the cost of maintenance as different compiler versions have different semantics and behavior.

Mitigation: Use the same compiler range for all contracts.

Impact: Informational/Low risk

Contracts should use the Upgradeable variant of OpenZeppelin Contracts

Summary: vToken.sol imports the non-upgradeable version of SafeERC20.

Details: If your contract is going to be deployed with upgradeability, such as using the OpenZeppelin Upgrades Plugins, it is recommended to use the Upgrade Safe variant of OpenZeppelin Contracts.

In particular, the contract use the non-upgradeable versions of SafeERC20.sol, IAccessControl.sol and Math.sol.

Mitigation: Use the upgradeable version of SafeERC20.sol, IAccessControl.sol and Math.sol.

Impact: Informational/Low risk

Misleading calculation of gap size

Summary: Length of __gap array is non-default in vToken.sol

Details: OpenZeppelin upgradeable contracts includes a state variable named __gap. This is an empty reserved space in storage that is put in place to allow to freely add new state variables in the future without compromising the storage compatibility with existing deployments. The size of the __gap array is calculated so that the amount of storage used by a contract always adds up to the same number (per default, 50 storage slots, unless a directive @custom:storage-size X is used with X being the number of storage slots).

It is important to note that constants variables does not count for gap calculation in OpenZeppelin upgradeable contracts (see e.g. ReentrancyGuardUpgradeable.sol), so the __gap array uses a non-default length for the __gap array and this is not explicitly noted in the code.

Mitigation: To maintain compatibility with OpenZeppelin library, use uint256[45] private __gap; instead of uint256[42] private __gap;.

Impact: Informational/Low risk

Pre-incrementing (++i) a variable is cheaper than post-incrementing (i++) it.

In FullMath.sol, change L124 to

++result;

Change usage of ≥ for >

EVM instruction set does not have an opcode for less than or equal, so L65 of UniswapV2PriceOracle.sol can be changed to if (timeElapsed > MIN_UPDATE_INTERVAL) if L19 is changed to uint private constant MIN_UPDATE_INTERVAL = 24 hours + 1;.