Velodrome Finance contest - sorrynotsorry's results

QA (LOW & NON-CRITICAL)

• Floating Pragma used in L2Governor.sol, L2GovernorCountingSimple.sol , L2GovernorVotesQuorumFraction.sol , Relayer.sol. Contracts should be deployed with the same compiler version and flags that they have been tested with thoroughly. Locking the pragma (i.e. by not using ^) helps to ensure that contracts do not accidentally get deployed using, for example, an outdated compiler version that might introduce bugs that affect the contract system negatively. Reference

• transfer and safeTransfer methods are used inside the codebase. Since these methods use 2300 gas stipend which is not adjustable, it may likely to get broken when calling a contract's fallback function. Reference Link -1, Reference Link -2

• The whole project have different solidity compiler ranges (0.8.0 - 0.8.13) referenced. This 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.

• Pair.sol, Router.sol, VotingEscrow.sol, PairFactory.sol, RedemptionSender.sol, use abi.encodePacked(). Using abi.encodePacked() with multiple variable length arguments can, in certain situations, lead to a hash collision. Instead abi.encode() can be used. Reference Link

• The project uses deprecated safeApprove in ChainlinkOracleClient.sol. Link

• The contract uses ecrecover() function at Pair.sol , VotingEscrow.sol. EVM's ecrecover is susceptible to signature malleability which allows replay attacks, but using OpenZeppelin’s ECDSA library can be mitigation for permit() and delegateBySig() function. Reference

• block.timestamp is used on many places at the scoped contracts. However, this can be manipulated by malicious miners like the options can be shown as expired before the end of the auction. Reference

• The codebase uses isContract() function in VotingEscrow.sol which should not be relied on if the target is a contract inside the construction. Reference is here

• The codebase has //TODO's in Minter.sol, VelodromeLibrary.sol, VotingEscrow.sol. The team might consider to remove them.

• The codebase is having lack of NatSpec comments such as @notice @dev @param on many places.It is recommended that Solidity contracts are fully annotated using NatSpec for all public interfaces (everything in the ABI) Reference

• Many require statement in Bribe.sol, Gauge.sol, Minter.sol, Pair.sol, PairFees.sol, RewardDistributor.sol, Router.sol, Velo.sol, VeloGovernor.sol, Voter.sol, VotingEscrow.sol, GaugeFactory.sol, PairFactory.sol, L2Governor.sol, L2GovernorCountingSimple.sol , L2GovernorVotesQuorumFraction.sol don't throw error. In case of any error pops up, it will not be possible to know the error source.

• In Voter.sol#L198, the approve function is used instead of the safeApprove function since the contract is not inherited from safeERC20. Tokens not compliant with the ERC20 specification could return false from the approve function call to indicate the approval fails, while the calling contract would not notice the failure if the return value is not checked.

• An expensive loop used by using SSTORE's at at Gauge.sol getReward() function Incrementing state_variable in a loop incurs a lot of gas because of expensive SSTOREs, which might lead to an out-of-gas and halting the process. Reference

• Router.sol uses create2 function of Solidity for the salted creation. However, there are some peculiarities in relation to salted creation. A contract can be re-created at the same address after having been destroyed. Yet, it is possible for that newly created contract to have a different deployed bytecode even though the creation bytecode has been the same (which is a requirement because otherwise the address would change). This is due to the fact that the constructor can query external state that might have changed between the two creations and incorporate that into the deployed bytecode before it is stored. Reference

• There are implicit conversations like in VotingEscrow.sol's tokenURI()function or RewardsDistributor.sol's ve_for_at() function as below;

return Math.max(uint(int256(pt.bias - pt.slope * (int128(int256(_timestamp - pt.ts))))), 0);

Since OZ's safeCast library is not utilized, the conversion can be wrapped inside require() statement to avoid surprises