Backd contest - cccz's results

Lines of code

https://github.com/code-423n4/2022-04-backd/blob/main/backd/contracts/pool/EthPool.sol#L30

Vulnerability details

Impact

Same as https://github.com/code-423n4/2022-01-openleverage-findings/issues/75

For example, when user calls the redeem function of the EthPool contract to redeem native token, the whole user redeem is being handled with a payable.transfer() call.

This is unsafe as transfer has hard coded gas budget and can fail when the user is a smart contract.

Whenever the user either fails to implement the payable fallback function or cumulative gas cost of the function sequence invoked on a native token transfer exceeds 2300 gas consumption limit the native tokens sent end up undelivered and the corresponding user funds return functionality will fail each time.

And the same problem is found in withdraw function of VaultReserve contract

https://github.com/code-423n4/2022-04-backd/blob/main/backd/contracts/vault/VaultReserve.sol#L81

Proof of Concept

https://github.com/code-423n4/2022-04-backd/blob/main/backd/contracts/pool/EthPool.sol#L30 https://github.com/code-423n4/2022-04-backd/blob/main/backd/contracts/vault/VaultReserve.sol#L81 https://consensys.net/diligence/blog/2019/09/stop-using-soliditys-transfer-now/

Tools Used

None

Recommended Mitigation Steps

Using low-level call.value(amount) with the corresponding result check or using the OpenZeppelin Address.sendValue is advised:

https://github.com/OpenZeppelin/openzeppelin-contracts/blob/master/contracts/utils/Address.sol#L60

Lines of code

https://github.com/code-423n4/2022-04-backd/blob/main/backd/contracts/oracles/ChainlinkOracleProvider.sol#L55

Vulnerability details

Impact

On ChainlinkOracleProvider.sol and ChainlinkUsdWrapper.sol , we are using latestRoundData, but there is no check if the return value indicates stale data.

    function _ethPrice() private view returns (int256) {
        (, int256 answer, , , ) = _ethOracle.latestRoundData();
        return answer;
    }
...
    function getPriceUSD(address asset) public view override returns (uint256) {
        address feed = feeds[asset];
        require(feed != address(0), Error.ASSET_NOT_SUPPORTED);

        (, int256 answer, , uint256 updatedAt, ) = AggregatorV2V3Interface(feed).latestRoundData();

        require(block.timestamp <= updatedAt + stalePriceDelay, Error.STALE_PRICE);
        require(answer >= 0, Error.NEGATIVE_PRICE);

        uint256 price = uint256(answer);
        uint8 decimals = AggregatorV2V3Interface(feed).decimals();
        return price.scaleFrom(decimals);
    }

This could lead to stale prices according to the Chainlink documentation:

https://docs.chain.link/docs/historical-price-data/#historical-rounds https://docs.chain.link/docs/faq/#how-can-i-check-if-the-answer-to-a-round-is-being-carried-over-from-a-previous-round

Proof of Concept

https://github.com/code-423n4/2022-04-backd/blob/main/backd/contracts/oracles/ChainlinkOracleProvider.sol#L55 https://github.com/code-423n4/2022-04-backd/blob/main/backd/contracts/oracles/ChainlinkUsdWrapper.sol#L64

Tools Used

None

Recommended Mitigation Steps

    function _ethPrice() private view returns (int256) {
        (uint80 roundID, int256 answer, , uint256 timestamp, uint80 answeredInRound) = _ethOracle.latestRoundData();
        require(answeredInRound >= roundID, "Stale price");
        require(timestamp != 0,"Round not complete");
        require(answer > 0,"Chainlink answer reporting 0");
        return answer;
    }
...
    function getPriceUSD(address asset) public view override returns (uint256) {
        address feed = feeds[asset];
        require(feed != address(0), Error.ASSET_NOT_SUPPORTED);
        (uint80 roundID, int256 answer, , uint256 updatedAt, uint80 answeredInRound) = AggregatorV2V3Interface(feed).latestRoundData();
        require(answeredInRound >= roundID, "Stale price");
        require(answer > 0," Error.NEGATIVE_PRICE");
        require(block.timestamp <= updatedAt + stalePriceDelay, Error.STALE_PRICE);

        uint256 price = uint256(answer);
        uint8 decimals = AggregatorV2V3Interface(feed).decimals();
        return price.scaleFrom(decimals);
    }