Yield micro contest #1 - 0xRajeev's results

Handle

0xRajeev

Vulnerability details

Impact

solc version 0.8.3 and 0.8.4 fixed important bugs in the compiler. Using version 0.8.1 misses these fixes and may cause a vulnerability.

Proof of Concept

https://github.com/code-423n4/2021-08-yield/blob/4dc46470e616dd0cbd9db9b4742e36c4d809e02c/contracts/utils/token/ERC20Rewards.sol#L2

https://github.com/ethereum/solidity/releases/tag/v0.8.4: Solidity 0.8.4 fixes a bug in the ABI decoder. The release contains an important bugfix. See decoding from memory bug blog post for more details.

https://github.com/ethereum/solidity/releases/tag/v0.8.3: Solidity 0.8.3 is a bugfix release that fixes an important bug about how the optimizer handles the Keccak256 opcode. For details on the bug, please see the bug blog post.

Tools Used

Manual Analysis

Recommended Mitigation Steps

Consider upgrading to 0.8.3 or 0.8.4

Handle

0xRajeev

Vulnerability details

Impact

Few events are missing emits which prevents the intended data from being observed easily by off-chain interfaces.

Proof of Concept

https://github.com/code-423n4/2021-08-yield/blob/4dc46470e616dd0cbd9db9b4742e36c4d809e02c/contracts/yieldspace/Strategy.sol#L48-L49

Tools Used

Manual Analysis

Recommended Mitigation Steps

Add emits or remove event declarations.

Handle

0xRajeev

Vulnerability details

Impact

That cauldron_ parameter is not used here and ladle_.cauldron() is used instead. The Ladle constructor initializes its cauldron value and so the only way this could differ from the parameter is if the argument to this function is specified incorrectly.

Proof of Concept

https://github.com/code-423n4/2021-08-yield/blob/4dc46470e616dd0cbd9db9b4742e36c4d809e02c/contracts/yieldspace/Strategy.sol#L100

https://github.com/code-423n4/2021-08-yield/blob/4dc46470e616dd0cbd9db9b4742e36c4d809e02c/contracts/yieldspace/Strategy.sol#L106-L107

https://github.com/code-423n4/2021-08-yield/blob/4dc46470e616dd0cbd9db9b4742e36c4d809e02c/contracts/Ladle.sol#L33

Tools Used

Manual Analysis

Recommended Mitigation Steps

Either use parameter or remove it in favor of the value from ladle_.cauldron().

Handle

0xRajeev

Vulnerability details

Impact

The declaration order of state variables affects storage slot packing and gas impact from reads/writes of shared slots.

The 8B rewardsPeriod struct gets packed with the 20B rewardsToken and may result in increased gas usage at runtime due to masked reads/writes. Declaring it instead with uint128 start and end times will force it to occupy a full slot and result in gas savings.

Proof of Concept

https://github.com/code-423n4/2021-08-yield/blob/4dc46470e616dd0cbd9db9b4742e36c4d809e02c/contracts/utils/token/ERC20Rewards.sol#L37-L40

https://github.com/code-423n4/2021-08-yield/blob/4dc46470e616dd0cbd9db9b4742e36c4d809e02c/contracts/utils/token/ERC20Rewards.sol#L53-L54

Other declarations: https://github.com/code-423n4/2021-08-yield/blob/4dc46470e616dd0cbd9db9b4742e36c4d809e02c/contracts/yieldspace/Strategy.sol#L51-L65

Tools Used

Manual Analysis

Recommended Mitigation Steps

Reconsider the declaration order of state variables and the use of variables with size less than word size of 256-bits. Both these affect gas efficiency.

Handle

0xRajeev

Vulnerability details

Impact

Public functions need to copy their arguments from calldata to memory resulting in more bytecode and gas consumption. If functions are never called from within the contracts, they can be declared external in which case their parameters are always in calldata without being copied to memory. This results in gas savings. There are many such public functions that don’t appear to be called from within the contract.

Proof of Concept

https://github.com/code-423n4/2021-08-yield/blob/4dc46470e616dd0cbd9db9b4742e36c4d809e02c/contracts/utils/token/ERC20Rewards.sol#L74-L75

https://github.com/code-423n4/2021-08-yield/blob/4dc46470e616dd0cbd9db9b4742e36c4d809e02c/contracts/yieldspace/Strategy.sol#L100-L101

All functions declared in this range: https://github.com/code-423n4/2021-08-yield/blob/4dc46470e616dd0cbd9db9b4742e36c4d809e02c/contracts/yieldspace/Strategy.sol#L127-L252

Tools Used

Manual Analysis

Recommended Mitigation Steps

Change function visibility from public to external


Handle

0xRajeev

Vulnerability details

Impact

SLOADs cost 2100 gas for first time reads of state variables and then 100 gas for repeated reads in the context of a transaction (post Berlin fork). MLOADs cost 3 gas units. Therefore, caching state variable in local variables for repeated reads saves gas.

Proof of Concept

Examples of state variables that are read at the lines shown and also later in that same function:

rewardsPeriod: https://github.com/code-423n4/2021-08-yield/blob/4dc46470e616dd0cbd9db9b4742e36c4d809e02c/contracts/utils/token/ERC20Rewards.sol#L80

_totalSupply: https://github.com/code-423n4/2021-08-yield/blob/4dc46470e616dd0cbd9db9b4742e36c4d809e02c/contracts/utils/token/ERC20Rewards.sol#L107

nextPool: https://github.com/code-423n4/2021-08-yield/blob/4dc46470e616dd0cbd9db9b4742e36c4d809e02c/contracts/yieldspace/Strategy.sol#L163

ladle: https://github.com/code-423n4/2021-08-yield/blob/4dc46470e616dd0cbd9db9b4742e36c4d809e02c/contracts/yieldspace/Strategy.sol#L172

base: https://github.com/code-423n4/2021-08-yield/blob/4dc46470e616dd0cbd9db9b4742e36c4d809e02c/contracts/yieldspace/Strategy.sol#L180

pool (600 gas savings): https://github.com/code-423n4/2021-08-yield/blob/4dc46470e616dd0cbd9db9b4742e36c4d809e02c/contracts/yieldspace/Strategy.sol#L183

pool: https://github.com/code-423n4/2021-08-yield/blob/4dc46470e616dd0cbd9db9b4742e36c4d809e02c/contracts/yieldspace/Strategy.sol#L208

cached: https://github.com/code-423n4/2021-08-yield/blob/4dc46470e616dd0cbd9db9b4742e36c4d809e02c/contracts/yieldspace/Strategy.sol#L262

Tools Used

Manual Analysis

Recommended Mitigation Steps

Consider caching state variables in local variables.

Handle

0xRajeev

Vulnerability details

Impact

Event emits where there are equivalent local variables or parameters for state variables can save gas by using those instead of state variables because of the expensive SLOADs.

Proof of Concept

rewardsToken: https://github.com/code-423n4/2021-08-yield/blob/4dc46470e616dd0cbd9db9b4742e36c4d809e02c/contracts/utils/token/ERC20Rewards.sol#L97

delay: https://github.com/code-423n4/2021-08-yield/blob/4dc46470e616dd0cbd9db9b4742e36c4d809e02c/contracts/utils/TimeLock.sol#L51

Tools Used

Manual Analysis

Recommended Mitigation Steps

Use parameters or local variables instead of state variables in event emits

Handle

0xRajeev

Vulnerability details

Impact

Function parameters are passed in calldata. For external functions, these are simply read from calldata. But explicitly specifying memory location for such parameters will force their copying to memory resulting in extra bytecode and more gas. Leaving them in calldata will save gas.

Proof of Concept

https://github.com/code-423n4/2021-08-yield/blob/4dc46470e616dd0cbd9db9b4742e36c4d809e02c/contracts/utils/EmergencyBrake.sol#L45-L46

https://github.com/code-423n4/2021-08-yield/blob/4dc46470e616dd0cbd9db9b4742e36c4d809e02c/contracts/utils/EmergencyBrake.sol#L66-L67

https://github.com/code-423n4/2021-08-yield/blob/4dc46470e616dd0cbd9db9b4742e36c4d809e02c/contracts/utils/EmergencyBrake.sol#L77-L78

https://github.com/code-423n4/2021-08-yield/blob/4dc46470e616dd0cbd9db9b4742e36c4d809e02c/contracts/utils/EmergencyBrake.sol#L101-L102

https://github.com/code-423n4/2021-08-yield/blob/4dc46470e616dd0cbd9db9b4742e36c4d809e02c/contracts/utils/EmergencyBrake.sol#L116-L117

https://github.com/code-423n4/2021-08-yield/blob/4dc46470e616dd0cbd9db9b4742e36c4d809e02c/contracts/FYTokenFactory.sol#L21-L22

Tools Used

Manual Analysis

Recommended Mitigation Steps

Do not use memory data location specifier for external function parameters

Handle

0xRajeev

Vulnerability details

Impact

As noted in the code comment, peek and get functions are the same for this oracle. So we can change peek to public visibility and have get call peek instead of copying the same code here. Minor deployment cost savings but increase in readability/maintainability.

Proof of Concept

https://github.com/code-423n4/2021-08-yield/blob/4dc46470e616dd0cbd9db9b4742e36c4d809e02c/contracts/oracles/composite/CompositeMultiOracle.sol#L91

https://github.com/code-423n4/2021-08-yield/blob/4dc46470e616dd0cbd9db9b4742e36c4d809e02c/contracts/oracles/composite/CompositeMultiOracle.sol#L74-L128

Tools Used

Manual Analysis

Recommended Mitigation Steps

Replace two functions having the same code with a single function.

Handle

0xRajeev

Vulnerability details

Impact

The require check for decimals_ <= 18 is unnecessary given its set to 18 right above unless this needs to be obtained differently as hinted by the comment.

Proof of Concept

https://github.com/code-423n4/2021-08-yield/blob/4dc46470e616dd0cbd9db9b4742e36c4d809e02c/contracts/oracles/compound/CTokenMultiOracle.sol#L110-L111

Tools Used

Manual Analysis

Recommended Mitigation Steps

Evaluate check and remove.

Handle

0xRajeev

Vulnerability details

Impact

The check for array lengths is unnecessary in two places where the following check on txHash will anyway fail if the lengths don’t match with what was hashed earlier during schedule. Removing the length check can save a little gas.

Such a require() in cancel() can be removed because if there is a mismatch, the entry lookup in transactions[] will fail anyway and also, this will not be sceduled/executed.

Proof of Concept

https://github.com/code-423n4/2021-08-yield/blob/4dc46470e616dd0cbd9db9b4742e36c4d809e02c/contracts/utils/TimeLock.sol#L70-L72

https://github.com/code-423n4/2021-08-yield/blob/4dc46470e616dd0cbd9db9b4742e36c4d809e02c/contracts/utils/TimeLock.sol#L81-L85

Tools Used

Manual Analysis

Recommended Mitigation Steps

Evaluate and remove these checks.

Handle

0xRajeev

Vulnerability details

Impact

setRewards() is missing input validation on parameters start and end to check if end > start. If accidentally set incorrectly, this will allow resetting new rewards while there is an ongoing one.

Proof of Concept

https://github.com/code-423n4/2021-08-yield/blob/4dc46470e616dd0cbd9db9b4742e36c4d809e02c/contracts/utils/token/ERC20Rewards.sol#L74-L88

Tools Used

Manual Analysis

Recommended Mitigation Steps

Add a require() to check that end > start.