Backd contest - 0xkatana's results

Originally submitted by warden 0xkatana in https://github.com/code-423n4/2022-04-backd-findings/issues/63, duplicate of https://github.com/code-423n4/2022-04-backd-findings/issues/17.

ChainLink latestRoundData data may be stale

Impact

The Chainlink API latestRoundData function returns price data with other timestamp and round data. The timestamp and round data should be validated to confirm the data is not stale.

Proof of concept

Places where latestRoundData is used https://github.com/code-423n4/2022-04-backd/blob/main/backd/contracts/oracles/ChainlinkUsdWrapper.sol#L55 https://github.com/code-423n4/2022-04-backd/blob/main/backd/contracts/oracles/ChainlinkUsdWrapper.sol#L64

Tools Used

Manual analysis

Recommended Mitigation Steps

When using the latestRoundData function, the return data much be checked for a stale price or an incomplete round.

[L-01] No checks for theoretical casting overflow

Impact

There is no protection around a uint256 to uint128 casting overflow. Although an overflow is unlikely if the curveIndex is incremented sequentially from zero (which is not necessarily a given), it is possible and no protections are in place.

Proof of concept

The same overflow pattern exists in several places https://github.com/code-423n4/2022-04-backd/blob/main/backd/contracts/strategies/BkdEthCvx.sol#L109 https://github.com/code-423n4/2022-04-backd/blob/main/backd/contracts/strategies/BkdTriHopCvx.sol#L214 https://github.com/code-423n4/2022-04-backd/blob/main/backd/contracts/strategies/BkdTriHopCvx.sol#L367

Tools Used

Manual analysis

Recommended Mitigation Steps

Import the OpenZeppelin SafeCast library or perform a check that the uint256 value is not greater than type(int128).max before casting

[L-02] ChainLink latestRoundData data may be stale

Impact

The Chainlink API latestRoundData function returns price data with other timestamp and round data. The timestamp and round data should be validated to confirm the data is not stale.

Proof of concept

Places where latestRoundData is used https://github.com/code-423n4/2022-04-backd/blob/main/backd/contracts/oracles/ChainlinkUsdWrapper.sol#L55 https://github.com/code-423n4/2022-04-backd/blob/main/backd/contracts/oracles/ChainlinkUsdWrapper.sol#L64

Tools Used

Manual analysis

Recommended Mitigation Steps

When using the latestRoundData function, the return data much be checked for a stale price or an incomplete round.

[L-03] Transfer function may revert if out of gas

Impact

Using the transfer function to send ether is no longer a recommended approach to send ether. EIP1884 can lead to the transfer reverting due to an out of gas condition. Even if the Backd vault does not use more than 2300 gas currently, the vault is a parameter that can be changed, which can cause problems on future changes.

Proof of concept

The _doTransferOut function sends ether to a payable address using the transfer function https://github.com/code-423n4/2022-04-backd/blob/main/backd/contracts/pool/EthPool.sol#L30

The _doTransferOut function is called in every redeem and vault rebalance https://github.com/code-423n4/2022-04-backd/blob/main/backd/contracts/pool/LiquidityPool.sol#L567 https://github.com/code-423n4/2022-04-backd/blob/main/backd/contracts/pool/LiquidityPool.sol#L754

The problem is that the receiving contract can execute code upon receiving ether (similar to the standard reentrancy pattern), but very little gas is forwarded with transfer. This can lead to unintended reverts. https://consensys.net/diligence/blog/2019/09/stop-using-soliditys-transfer-now/

Tools Used

Manual analysis

Recommended Mitigation Steps

Because the vault address should be a trusted address controlled by Backd, use call instead of transfer

[L-04] safeApprove is deprecated

Impact

The safeApprove function is deprecated according to the OpenZeppelin comment Deprecated. This function has issues similar to the ones found in {IERC20-approve}, and its usage is discouraged.

Although the solmate implementation is used, the same "double withdrawal" front-running problem exists as the OpenZeppelin implementation where both the old and new allowance could be used https://docs.openzeppelin.com/contracts/4.x/api/token/erc20#IERC20-approve-address-uint256-

Proof of concept

safeApprove instances in code https://github.com/code-423n4/2022-04-backd/blob/main/backd/contracts/CvxCrvRewardsLocker.sol#L53-L62 https://github.com/code-423n4/2022-04-backd/blob/main/backd/contracts/tokenomics/AmmConvexGauge.sol#L62 https://github.com/code-423n4/2022-04-backd/blob/main/backd/contracts/strategies/BkdTriHopCvx.sol#L71-L73 https://github.com/code-423n4/2022-04-backd/blob/main/backd/contracts/strategies/BkdTriHopCvx.sol#L129-L132

This Open Zeppelin comment indicates it is not recommended https://github.com/OpenZeppelin/openzeppelin-contracts/blob/master/contracts/token/ERC20/utils/SafeERC20.sol#L39-L43

Tools Used

Manual analysis

Recommended Mitigation Steps

Replace safeApprove with {safeIncreaseAllowance} or {safeDecreaseAllowance} instead

[G-01] Redundant zero initialization

Solidity does not recognize null as a value, so uint variables are initialized to zero. Setting a uint variable to zero is redundant and can waste gas.

There are several places where an int is initialized to zero, which looks like

uint256 i = 0;

Some of the many instances of this in code https://github.com/code-423n4/2022-04-backd/blob/main/backd/contracts/BkdLocker.sol#L133 https://github.com/code-423n4/2022-04-backd/blob/main/backd/contracts/BkdLocker.sol#L310 https://github.com/code-423n4/2022-04-backd/blob/main/backd/contracts/Controller.sol#L114 https://github.com/code-423n4/2022-04-backd/blob/main/backd/contracts/Controller.sol#L117 https://github.com/code-423n4/2022-04-backd/blob/main/backd/contracts/StakerVault.sol#L144 https://github.com/code-423n4/2022-04-backd/blob/main/backd/contracts/StakerVault.sol#L260 https://github.com/code-423n4/2022-04-backd/blob/main/backd/contracts/tokenomics/InflationManager.sol#L91 https://github.com/code-423n4/2022-04-backd/blob/main/backd/contracts/tokenomics/InflationManager.sol#L105 https://github.com/code-423n4/2022-04-backd/blob/main/backd/contracts/tokenomics/InflationManager.sol#L109 https://github.com/code-423n4/2022-04-backd/blob/main/backd/contracts/tokenomics/InflationManager.sol#L114 https://github.com/code-423n4/2022-04-backd/blob/main/backd/contracts/tokenomics/InflationManager.sol#L166 https://github.com/code-423n4/2022-04-backd/blob/main/backd/contracts/tokenomics/InflationManager.sol#L191 https://github.com/code-423n4/2022-04-backd/blob/main/backd/contracts/tokenomics/InflationManager.sol#L259 https://github.com/code-423n4/2022-04-backd/blob/main/backd/contracts/tokenomics/InflationManager.sol#L283 https://github.com/code-423n4/2022-04-backd/blob/main/backd/contracts/tokenomics/InflationManager.sol#L357 https://github.com/code-423n4/2022-04-backd/blob/main/backd/contracts/tokenomics/InflationManager.sol#L381 https://github.com/code-423n4/2022-04-backd/blob/main/backd/contracts/tokenomics/InflationManager.sol#L404 https://github.com/code-423n4/2022-04-backd/blob/main/backd/contracts/tokenomics/InflationManager.sol#L445

Recommended Mitigation Steps

Remove the redundant zero initialization uint256 i;

[G-02] Use prefix not postfix in loops

Using a prefix increment (++i) instead of a postfix increment (i++) saves gas for each loop cycle and so can have a big gas impact when the loop executes on a large number of elements.

There are many examples of this https://github.com/code-423n4/2022-04-backd/blob/main/backd/contracts/BkdLocker.sol#L310 https://github.com/code-423n4/2022-04-backd/blob/main/backd/contracts/Controller.sol#L117 https://github.com/code-423n4/2022-04-backd/blob/main/backd/contracts/tokenomics/InflationManager.sol#L91 https://github.com/code-423n4/2022-04-backd/blob/main/backd/contracts/tokenomics/InflationManager.sol#L105 https://github.com/code-423n4/2022-04-backd/blob/main/backd/contracts/tokenomics/InflationManager.sol#L109 https://github.com/code-423n4/2022-04-backd/blob/main/backd/contracts/tokenomics/InflationManager.sol#L114 https://github.com/code-423n4/2022-04-backd/blob/main/backd/contracts/tokenomics/InflationManager.sol#L166 https://github.com/code-423n4/2022-04-backd/blob/main/backd/contracts/tokenomics/InflationManager.sol#L191 https://github.com/code-423n4/2022-04-backd/blob/main/backd/contracts/tokenomics/InflationManager.sol#L259 https://github.com/code-423n4/2022-04-backd/blob/main/backd/contracts/tokenomics/InflationManager.sol#L283 https://github.com/code-423n4/2022-04-backd/blob/main/backd/contracts/tokenomics/InflationManager.sol#L357 https://github.com/code-423n4/2022-04-backd/blob/main/backd/contracts/tokenomics/InflationManager.sol#L381 https://github.com/code-423n4/2022-04-backd/blob/main/backd/contracts/tokenomics/InflationManager.sol#L404 https://github.com/code-423n4/2022-04-backd/blob/main/backd/contracts/tokenomics/InflationManager.sol#L445

Recommended Mitigation Steps

Use prefix not postfix to increment in a loop

[G-03] Short require strings save gas

Strings in solidity are handled in 32 byte chunks. A require string longer than 32 bytes uses more gas. Shortening these strings will save gas.

Several cases of this gas optimization was found https://github.com/code-423n4/2022-04-backd/blob/c856714a50437cb33240a5964b63687c9876275b/backd/libraries/Errors.sol#L11 https://github.com/code-423n4/2022-04-backd/blob/c856714a50437cb33240a5964b63687c9876275b/backd/libraries/Errors.sol#L18 https://github.com/code-423n4/2022-04-backd/blob/c856714a50437cb33240a5964b63687c9876275b/backd/libraries/Errors.sol#L22 https://github.com/code-423n4/2022-04-backd/blob/c856714a50437cb33240a5964b63687c9876275b/backd/libraries/Errors.sol#L24 https://github.com/code-423n4/2022-04-backd/blob/c856714a50437cb33240a5964b63687c9876275b/backd/libraries/Errors.sol#L26 https://github.com/code-423n4/2022-04-backd/blob/c856714a50437cb33240a5964b63687c9876275b/backd/libraries/Errors.sol#L28 https://github.com/code-423n4/2022-04-backd/blob/c856714a50437cb33240a5964b63687c9876275b/backd/libraries/Errors.sol#L30 https://github.com/code-423n4/2022-04-backd/blob/c856714a50437cb33240a5964b63687c9876275b/backd/libraries/Errors.sol#L61

Recommended Mitigation Steps

Shorten all require strings to less than 32 characters

[G-04] Use != 0 instead of > 0

Using > 0 uses slightly more gas than using != 0. Use != 0 when comparing uint variables to zero, which cannot hold values below zero

Locations where this was found include https://github.com/code-423n4/2022-04-backd/blob/main/backd/contracts/BkdLocker.sol#L90 https://github.com/code-423n4/2022-04-backd/blob/main/backd/contracts/BkdLocker.sol#L91 https://github.com/code-423n4/2022-04-backd/blob/main/backd/contracts/BkdLocker.sol#L136 https://github.com/code-423n4/2022-04-backd/blob/main/backd/contracts/BkdLocker.sol#L253 https://github.com/code-423n4/2022-04-backd/blob/main/backd/contracts/BkdLocker.sol#L300 https://github.com/code-423n4/2022-04-backd/blob/main/backd/contracts/BkdLocker.sol#L308 https://github.com/code-423n4/2022-04-backd/blob/main/backd/contracts/LpToken.sol#L87 https://github.com/code-423n4/2022-04-backd/blob/main/backd/contracts/CvxCrvRewardsLocker.sol#L173 https://github.com/code-423n4/2022-04-backd/blob/main/backd/contracts/CvxCrvRewardsLocker.sol#L261

Recommended Mitigation Steps

Replace > 0 with != 0 to save gas

[G-05] Cache array length before loop

Caching the array length outside a loop saves reading it on each iteration, as long as the array's length is not changed during the loop. This saves gas.

This optimization is already used in most places, but is not used in these places https://github.com/code-423n4/2022-04-backd/blob/main/backd/contracts/tokenomics/VestedEscrow.sol#L93 https://github.com/code-423n4/2022-04-backd/blob/main/backd/contracts/tokenomics/InflationManager.sol#L109 https://github.com/code-423n4/2022-04-backd/blob/main/backd/contracts/strategies/ConvexStrategyBase.sol#L313 https://github.com/code-423n4/2022-04-backd/blob/main/backd/contracts/strategies/ConvexStrategyBase.sol#L380

Recommended Mitigation Steps

Cache the array length before the for loop

[G-06] Split up require statements instead of &&

Combining require statement conditions with && logic uses unnecessary gas. It is better to split up each part of the logical statement into a separate require statements

Several places had require statements with many logical "and"s. Instead, split into two to save gas https://github.com/code-423n4/2022-04-backd/blob/main/backd/contracts/SwapperRegistry.sol#L35-L39 https://github.com/code-423n4/2022-04-backd/blob/main/backd/contracts/swappers/Swapper3Crv.sol#L59 https://github.com/code-423n4/2022-04-backd/blob/main/backd/contracts/actions/topup/TopUpAction.sol#L676

Recommended Mitigation Steps

Use separate require statements instead of concatenating with &&

[G-07] Use simple comparison in if statement

The comparison operators >= and <= use more gas than >, <, or ==. Replacing the >= and ≤ operators with a comparison operator that has an opcode in the EVM saves gas

There are many places where this improvement applies, but one example is https://github.com/code-423n4/2022-04-backd/blob/main/backd/contracts/tokenomics/LpGauge.sol#L60-L63

if (amount <= 0) return 0;
perUserShare[beneficiary] = 0;
_mintRewards(beneficiary, amount);
return amount;

A simple comparison can be used for gas savings by reversing the logic

if (amount > 0) {
perUserShare[beneficiary] = 0;
_mintRewards(beneficiary, amount);
return amount;
} else {
return 0;
}

Recommended Mitigation Steps

Replace the comparison operator and reverse the logic to save gas using the suggestions above

[G-08] minter can be immutable

The minter state variable can only be set once in setMinter function and can be immutable. If this variable is immutable, a require check can be removed.

minter is declared in line 28 https://github.com/code-423n4/2022-04-backd/blob/main/backd/contracts/tokenomics/InflationManager.sol#L28

minter is only set once in setMinter https://github.com/code-423n4/2022-04-backd/blob/main/backd/contracts/tokenomics/InflationManager.sol#L59

Recommended Mitigation Steps

Declare minter immutable with address public immutable minter;. Then remove this require check because it is not necessary if the variable can only be set once https://github.com/code-423n4/2022-04-backd/blob/main/backd/contracts/tokenomics/InflationManager.sol#L57