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:
EthPool's _doTransferOut that employ payable(to).transfer(amount) is also called with Vault as recipient in LiquidityPool's depositFor, executeNewRequiredReserves(), executeNewReserveDeviation() -> _rebalanceVault() sequence:
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:
chase-manning
commented
2 years ago
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
transferhas hard coded gas budget and can fail when thetois a smart contract. Such transactions will fail for smart contract users which don't fit to 2300 gas stipendtransferhave.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 OpenZeppelinAddress.sendValue:https://github.com/OpenZeppelin/openzeppelin-contracts/blob/master/contracts/utils/Address.sol#L60