When the value of the post-loop increment/decrement is not stored or used in any calculations, the prefix increment/decrement operators (++i/--i) cost less gas (five) PER LOOP than the postfix increment/decrement operators (i++/i--). If the account has 20 or more vaults, each subsequent call to addVault() will cost more than 100 gas. The same issue is present for removeVault() and for _getDepositedBalance() in ConvexYieldWrapper.sol, and is present in ConvexStakingWrapper.sol's addRewards(), _calCvxIntegral(), _calRewardIntegral(), _checkpoint(), _checkpointAndClaim(), and earned(), for a total of nine instances of three extra gas per loop, each. If a user has only a few vaults, but calls a few of these functions, the gas used will be more than 100.
when the line remains as is, and the contract is compiled and deployed using hardhat (with settings {"compiler": { version: "0.8.6", settings: { optimizer: { enabled: true, runs: 200000 } } } }), the contract has the following 'bad' deployed bytecode:
As compared to the good version, bad version contains an extra DUP1 (costing 3 extra gas) at program counter 3879, has a SWAP2 at pc 3889 versus good's SWAP1 (same cost), and the bad version has an extra POP (costing 2 extra gas) at pc 3891.
Handle
IllIllI
Vulnerability details
Impact
When the value of the post-loop increment/decrement is not stored or used in any calculations, the prefix increment/decrement operators (
++i
/--i
) cost less gas (five) PER LOOP than the postfix increment/decrement operators (i++
/i--
). If the account has 20 or more vaults, each subsequent call toaddVault()
will cost more than 100 gas. The same issue is present forremoveVault()
and for_getDepositedBalance()
in ConvexYieldWrapper.sol, and is present in ConvexStakingWrapper.sol'saddRewards()
,_calCvxIntegral()
,_calRewardIntegral()
,_checkpoint()
,_checkpointAndClaim()
, andearned()
, for a total of nine instances of three extra gas per loop, each. If a user has only a few vaults, but calls a few of these functions, the gas used will be more than 100.Proof of Concept
Taking this line as an example:
https://github.com/code-423n4/2022-01-yield/blob/e946f40239b33812e54fafc700eb2298df1a2579/contracts/ConvexYieldWrapper.sol#L63
when the line remains as is, and the contract is compiled and deployed using hardhat (with settings
{"compiler": { version: "0.8.6", settings: { optimizer: { enabled: true, runs: 200000 } } } }
), the contract has the following 'bad' deployed bytecode:When the line is changed to use a prefix increment:
for (uint256 i = 0; i < vaultsLength; ++i) {
it has the following 'good' deployed bytecode:
Drilling down to the opcodes in the specific for-loop, we see
with
i++
(bad):and with
++i
(good):As compared to the good version, bad version contains an extra
DUP1
(costing 3 extra gas) at program counter 3879, has aSWAP2
at pc 3889 versus good'sSWAP1
(same cost), and the bad version has an extraPOP
(costing 2 extra gas) at pc 3891.Tools Used
Hardhat npx @remix-project/remix-lib
Recommended Mitigation Steps
Use
++i
rather thani++
in all places