sherlock-audit / 2022-09-notional-judging

4 stars 2 forks source link

ethan-crypto - Medium: Sending ether with call to exitVault to cover shortfall in cash from lending eth can result in revert for contract accounts. #107

Closed sherlock-admin closed 1 year ago

sherlock-admin commented 1 year ago

ethan-crypto

medium

Medium: Sending ether with call to exitVault to cover shortfall in cash from lending eth can result in revert for contract accounts.

Summary

In the case where a contract account calls exitVault and needs to send ether to cover shortfall in cash from lending eth, it uses the .transfer() method inside the GenericToken.transferNativeTokenOut() library wrapper method to transfer out unused msg.value. (e.i. even if there is no unused part of msg.value it still calls the transfer method sending 0 ether back).

Vulnerability Detail

transfer() uses a fixed amount of gas, which was used to prevent reentrancy. However this restricts contract accounts from being able to repay debts from their accounts directly. Specifically contracts that need more than 2300 units of gas in their receive()/fallback() methods to process the transaction.

Impact

transfer() uses a fixed amount of gas, which can result in revert. https://consensys.net/diligence/blog/2019/09/stop-using-soliditys-transfer-now/

Code Snippet

https://github.com/notional-finance/contracts-v2/blob/cf05d8e3e4e4feb0b0cef2c3f188c91cdaac38e0/contracts/internal/vaults/VaultConfiguration.sol#L638

Tool used

Manual Review

Recommendation

Use call instead of transfer()

Duplicate of #63