Backd contest - sorrynotsorry's results

Lines of code

https://github.com/code-423n4/2022-04-backd/blob/c856714a50437cb33240a5964b63687c9876275b/backd/contracts/oracles/ChainlinkUsdWrapper.sol#L56

Vulnerability details

Impact

Chainlink's latestRoundData might return stale or incorrect results.

Proof of Concept

Chainlink's latestRoundData might return stale or incorrect results If there is a problem with Chainlink starting a new round and finding consensus on the new value for the oracle (e.g. Chainlink nodes abandon the oracle, chain congestion, vulnerability/attacks on the chainlink system) consumers of this contract may continue using outdated stale or incorrect data (if oracles are unable to submit no new round is started).

https://github.com/code-423n4/2022-04-backd/blob/c856714a50437cb33240a5964b63687c9876275b/backd/contracts/oracles/ChainlinkUsdWrapper.sol#L56

Reference

Tools Used

Manual Review

Recommended Mitigation Steps

The team can consider adding checks on the return data with proper revert messages if the price is stale or the round is incomplete.

QA (LOW / NON-CRITICAL)

• No address(0) or Zero value check at the constructors/initiliazers of Authorization.sol , Vault.sol, ETHVault.sol, ERC20Vault.sol, StakerVault.sol,TopUpAction.sol and many other contracts.

• At Vault.sol#233, set() method is inherited inside executeNewStrategy(). It requires 3 params but only 2 params used. It will fail and lock the funds inside if there are any funds left due to locking deadlines.

• At Vault.sol#L323, logic should be;

if (totalDebt > debtLimitAllocated)

instead of;

if (totalDebt >= debtLimitAllocated)

• transfer method is used severalplacesinside the codebase. Since transfer method uses 2300 gas stipend which is not adjustable,it may likely to get broken in future in case of hardforks causing gas price changes or when calling a contract's fallback function. Reference Link -1, Reference Link -2

• There is a function overloading starting with AddressProvider.sol' allActions() method. allActions() returns _actions.toArray() for where it calls toArray(EnumerableSet.Bytes32Set storage values) of EnumerableExtensions.sol. However, this function affects the state changes and state changes should not be based upon it as stated here due to function has unbounded cost.

• At TopUpAction.sol#L943 , TopUpActipnFeeHandler.sol#L232 , CTokenRegistry.sol#L67, 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

• Team can consider to remove TODO's at TopUpAction.sol#L713 , ConvexStrategyBase.sol#L4-L5

• Usage of deprecated safeApprove. Link

• Typo at ChainlinkOracleProvider.sol#L37 , Correct wording should be stale insted of stake which is confusing

GAS OPTIMIZATIONS

• Following math operations can be carried out unchecked since the conditions are checked on previous LOCs and will not underflow; Vault.sol#L125 - Condition checked on Vault.sol#L122 Vault.sol#L130-L131 - Condition checked on Vault.sol#L127 Vault.sol#L141 - Condition checked on Vault.sol#L122-L134 Vault.sol#L423 - Condition checked on Vault.sol#L421 Vault.sol#L440 - Condition checked on Vault.sol#L437 Vault.sol#L444 - Condition checked on Vault.sol#L443 Vault.sol#L452 - Condition checked on Vault.sol#L451 Vault.sol#L482 - Condition checked on the same line Vault.sol#L591 - Condition checked on Vault.sol#L589 Vault.sol#L595 - Condition checked on Vault.sol#L593 Vault.sol#L600 - Condition checked on Vault.sol#L599 Vault.sol#L605 - Condition checked on Vault.sol#L603 Vault.sol#L637 - Condition checked on Vault.sol#L635 Vault.sol#L688 - Profit condition Vault.sol#L695 - Condition checked on Vault.sol#L692-L693 Vault.sol#L781 - Condition checked on the same line as a ternary operator

• Vault.sol#L581 calls the function again inside same contract creating unnecessary function overloading (strategy.harvest();). Since it's not inheriting a function in outer contracts, function logic can be inlined directly in this line in order to save gas.

• At Vault.sol#L323, _handleExcessDebt() is called which initiates a function triggering inside the same contract (not inheriting other contracts). However, the function logic can be inlined to executeDebtLimit() by passing required params/state variables to prevent SLOADs.

• Caching .length inside loop functions will save gas, located at StakerVault.sol#L260 , RoleManager.sol#L80 ,TopUpActipn.sol#188 , TopUpKeeperHelper.sol#L43 , TopUpKeeperHelper.sol#L46 , TopUpKeeperHelper.sol#L72 , CompoundHandler.sol#135 , CTokenRegistry.sol#L61 , ConvexStrategyBase.sol#L313 , ConvexStrategyBase.sol#L380

• Using unchecked {++i} instead of i++ will save gas.

• Using immutable instead of constant variables will save gas.

• Choosing either named return or explicit instead of specifying both may reduce gas due to unnecessary bytecode introduced. There is inconsistent use of implicit named return variables across the entire codebase which makes readability and maintainability hard.

• Use bytes32 instead of string to save gas whenever possible. String is a dynamic data structure and therefore is more gas consuming then bytes32. You could use bytes32 instead of string in the following places: LiquidityPool.sol#L71 , PoolFactory.sol#L47-L48