Backd contest - hyh's results

Lines of code

https://github.com/code-423n4/2022-04-backd/blob/main/backd/contracts/strategies/BkdEthCvx.sol#L77

Vulnerability details

Impact

These contracts use payable.transfer for internal fund transfer, where the recipients are vaults, pools, strategies.

This is generally unsafe as transfer has hard coded gas budget and can fail when the to is a smart contract. Such transactions will fail for smart contract users which don't fit to 2300 gas stipend transfer have.

Setting severity to be medium as while at any point of time this can be controlled by having low-gas receiving functions there is a possibility that such a Vault/Pool/Strategy can be introduced later (and gas cost of particular functions can change), so the system isn't safe in this regard. Using such a contract will make the functions described below unavailable.

Proof of Concept

BkdEthCvx._withdraw transfers to Vault with payable.transfer:

https://github.com/code-423n4/2022-04-backd/blob/main/backd/contracts/strategies/BkdEthCvx.sol#L77

BkdEthCvx._withdrawAll does the same:

https://github.com/code-423n4/2022-04-backd/blob/main/backd/contracts/strategies/BkdEthCvx.sol#L117

VaultReserve employ the similar approach:

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

ETHVault's _transfer called by Vault uses payable.transfer for transfer to Pool:

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

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

EthPool's _doTransferOut that employ payable(to).transfer(amount) is also called with Vault as recipient in LiquidityPool's depositFor, executeNewRequiredReserves(), executeNewReserveDeviation() -> _rebalanceVault() sequence:

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

Recommended Mitigation Steps

Consider introducing direct non-reentrancy modifiers and then using low-level call.value(amount) with the corresponding result check or using the OpenZeppelin Address.sendValue:

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#L58

Vulnerability details

Impact

As stale price is determined by time since last timestamp, the price that is most recent, but wasn't updated for more than 2 hours (say there were no trades on the market) will be rejected, which makes system functionality unavailable in such a case. This can be deemed too conservative, but that's an architectural choice.

However, in the same time real stale price will be accepted as long as it happens to be in the last 2 hours window. I.e. ChainlinkOracleProvider do not require the price to be the most recent, simply accepting any price given that it is not older than 2 hours.

Another issue is that zero price, which in the most cases will indicate Oracle malfunction, will be accepted by the system. I.e. devalued tokens usually do have positive, albeit very small, price.

Proof of Concept

getPriceUSD allows for zero price:

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

Oracle prices are used as a denominator across the system, for example:

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

https://github.com/code-423n4/2022-04-backd/blob/main/backd/contracts/strategies/StrategySwapper.sol#L319-L319

Recommended Mitigation Steps

Consider imposing checks for positive price and for the round id, timestamp fields:

https://docs.chain.link/docs/feed-registry/#latestrounddata

(int256 roundID, int256 priceInUsd, , int256 updatedAt, int256 answeredInRound) = baseAggregator.latestRoundData();

require(priceInUsd > 0 && updatedAt > 0 && answeredInRound >= roundID , "Price invalid");