JPEG'd contest - reassor's results

Lines of code

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

Vulnerability details

Impact

Contracts NFTVault.sol and FungibleAssetVaultForDAO use Chainlink's latestAnswer as an oracle for prices of multiple assets. This function will return the last value, but it is not possible to check if the provided data is fresh. In addition latestAnswer has been marked as deprecated and if at some point Chainlink stops supporting deprecated APIs the protocol will stop working and it will be required to redeploy contracts.

Proof of Concept

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

Tools Used

Manual Review / VSCode

Recommended Mitigation Steps

Use the latestRoundData function to retrieve prices of the assets. Add checks on the returned data with proper revert messages if the price is stale or the round is uncomplete, for example:

(
   roundId,
   rawPrice,
   ,
   updateTime,
   answeredInRound
 ) = AggregatorV3Interface(XXXXX).latestRoundData();
 require(rawPrice > 0, "price <= 0");
 require(updateTime != 0, "incomplete round");
 require(answeredInRound >= roundId, "stale price");

Lines of code

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

Vulnerability details

Impact

The Istanbul hardfork increases the gas cost of the SLOAD operation and therefore breaks some existing smart contracts.

Contract FungibleAssetVaultForDAO.sol for withdrawing ETH collateral uses the transfer function, which has a fixed gas stipend and can fail.

The reason behind this is that, after the Istanbul hardfork, any smart contract that uses transfer() or send() is taking a hard dependency on a gas costs by forwarding a fixed amount of gas (2300). This forwards 2300 gas, which may not be enough if the recipient is a contract and the cost of gas changes.

Proof of Concept

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

Recommended Mitigation Steps

It is recommended to use safe wrapper library, such as the OpenZeppelin Address library’s and its sendValue function that forwards sufficient gas for the transfer regardless of the underlying OPCODE gas costs.