code-423n4 / 2021-05-yield-findings

0 stars 0 forks source link

Missing zero-address validations
 #42

Open code423n4 opened 3 years ago

code423n4 commented 3 years ago

Handle

0xRajeev

Vulnerability details

Impact

While the codebase does a great job of input validation for parameters of all kinds and especially addresses, there are a few places where zero-address validations are missing. None of them are catastrophic, will result in obvious reverts and can be reset given the permissioned/controlled interactions with the contracts.

Nevertheless, it is helpful to add zero-address validations to be consistent and ensure high availability of the protocol with resistance to accidental misconfigurations.

Proof of Concept

  1. Spot oracle address:

https://github.com/code-423n4/2021-05-yield/blob/e4c8491cd7bfa5dc1b59eb1b257161cd5bf8c6b0/contracts/Cauldron.sol#L124-L127

https://github.com/code-423n4/2021-05-yield/blob/e4c8491cd7bfa5dc1b59eb1b257161cd5bf8c6b0/contracts/Wand.sol#L78

  1. Grab receiver addresses for other liquidation engines besides the Witch (which is fine because it uses address(this) for receiver):

https://github.com/code-423n4/2021-05-yield/blob/e4c8491cd7bfa5dc1b59eb1b257161cd5bf8c6b0/contracts/Cauldron.sol#L235-L240

https://github.com/code-423n4/2021-05-yield/blob/e4c8491cd7bfa5dc1b59eb1b257161cd5bf8c6b0/contracts/Cauldron.sol#L349-L360

https://github.com/code-423n4/2021-05-yield/blob/e4c8491cd7bfa5dc1b59eb1b257161cd5bf8c6b0/contracts/Witch.sol#L53

  1. Spot oracle’s spotSource in makeIlk() [unlike the checks in makeBase()]:

https://github.com/code-423n4/2021-05-yield/blob/e4c8491cd7bfa5dc1b59eb1b257161cd5bf8c6b0/contracts/Wand.sol#L76-L77

  1. Oracle setSource() in Compound, Uniswap and Chainlink can zero-address validate the source, instead of at the callers in Wand:

https://github.com/code-423n4/2021-05-yield/blob/e4c8491cd7bfa5dc1b59eb1b257161cd5bf8c6b0/contracts/oracles/compound/CompoundMultiOracle.sol#L22-L23

https://github.com/code-423n4/2021-05-yield/blob/e4c8491cd7bfa5dc1b59eb1b257161cd5bf8c6b0/contracts/oracles/uniswap/UniswapV3Oracle.sol#L49-L60

https://github.com/code-423n4/2021-05-yield/blob/e4c8491cd7bfa5dc1b59eb1b257161cd5bf8c6b0/contracts/oracles/chainlink/ChainlinkMultiOracle.sol#L29-L44

Tools Used

Manual Analysis

Recommended Mitigation Steps

Add require() to zero-address validate the address parameters.

alcueca commented 3 years ago

While these checks could be added, I don't think they are worth the gas. To me it seems something it should be checked in the frontend, or for governance actions, on spell review.