JPEG'd contest - Jujic's results

Lines of code

https://github.com/code-423n4/2022-04-jpegd/blob/e72861a9ccb707ced9015166fbded5c97c6991b6/contracts/vaults/FungibleAssetVaultForDAO.sol#L105

Vulnerability details

Impact

Price can be stale and can lead to wrong answer return value.

Proof of Concept

Oracle data feed is insufficiently validated. There is no check for stale price and round completeness. Price can be stale and can lead to wrong answer return value.

function _collateralPriceUsd() internal view returns (uint256) {
        int256 answer = oracle.latestAnswer();
        uint8 decimals = oracle.decimals();

        require(answer > 0, "invalid_oracle_answer");

        ...

https://github.com/code-423n4/2022-04-jpegd/blob/e72861a9ccb707ced9015166fbded5c97c6991b6/contracts/vaults/FungibleAssetVaultForDAO.sol#L105

Tools Used

Manual review

Recommended Mitigation Steps

Validate data feed

function _collateralPriceUsd() internal view returns (uint256) {

(uint80 roundID, int256 answer, , uint256 timestamp, uint80 answeredInRound) = oracle.latestRoundData();
   
    require(answer > 0, "invalid_oracle_answer");
    require(answeredInRound >= roundID, "ChainLink: Stale price");
    require(timestamp > 0, "ChainLink: Round not complete");

         ...

Lines of code

https://github.com/code-423n4/2022-04-jpegd/blob/e72861a9ccb707ced9015166fbded5c97c6991b6/contracts/vaults/FungibleAssetVaultForDAO.sol#L105

Vulnerability details

Impact

The Chainlink API (latestAnswer) used in the FungibleAssetVaultForDAO contract is deprecated. This function does not error if no answer has been reached but returns 0.

Proof of Concept

https://github.com/code-423n4/2022-04-jpegd/blob/e72861a9ccb707ced9015166fbded5c97c6991b6/contracts/vaults/FungibleAssetVaultForDAO.sol#L105

int256 answer = oracle.latestAnswer();

Tools Used

https://web.archive.org/web/20210304160150/https://docs.chain.link/docs/deprecated-aggregatorinterface-api-reference

Recommended Mitigation Steps

Use the latestRoundData function to get the price instead.

Lines of code

https://github.com/code-423n4/2022-04-jpegd/blob/e72861a9ccb707ced9015166fbded5c97c6991b6/contracts/vaults/FungibleAssetVaultForDAO.sol#L201

Vulnerability details

Impact

The use of transfer in withdraw() to send ether is now considered bad practice as gas costs can change which would break the code

Proof of Concept

https://github.com/code-423n4/2022-04-jpegd/blob/e72861a9ccb707ced9015166fbded5c97c6991b6/contracts/vaults/FungibleAssetVaultForDAO.sol#L201

if (collateralAsset == ETH) payable(msg.sender).transfer(amount);

Tools Used

https://consensys.net/diligence/blog/2019/09/stop-using-soliditys-transfer-now/

https://chainsecurity.com/istanbul-hardfork-eips-increasing-gas-costs-and-more/

Recommended Mitigation Steps

Recommend using call instead, and make sure to check for reentrancy.