sherlock-audit / 2022-10-merit-circle-judging

1 stars 0 forks source link

ignacio - CALL() SHOULD BE USED INSTEAD OF TRANSFER() #99

Closed sherlock-admin closed 2 years ago

sherlock-admin commented 2 years ago

ignacio

medium

CALL() SHOULD BE USED INSTEAD OF TRANSFER()

Summary

transfer() uses a fixed amount of gas, which was used to prevent reentrancy. However this limit your protocol to interact with others contracts that need more than that to process the transaction.

Vulnerability Detail

The use of the deprecated transfer() function for an address will inevitably make the transaction fail when:

The claimer smart contract does not implement a payable function. The claimer smart contract does implement a payable fallback which uses more than 2300 gas unit. The claimer smart contract implements a payable fallback function that needs less than 2300 gas units but is called through proxy, raising the call’s gas usage above 2300. Additionally, using higher than 2300 gas might be mandatory for some multisig wallets. 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. The impact would mean that any contracts receiving funds would potentially be unable to retrieve funds from the swap.

Recommended Mitigation Steps

use call() to send eth , re-entrancy has been accounted for in all functions that reference Solidity's transfer() . This has been done by using a re-entrancy guard, therefore, we can rely on msg.sender.call.value(amount)` or using the OpenZeppelin Address.sendValue library

Impact

Code Snippet

https://github.com/Merit-Circle/merit-liquidity-mining/blob/ce5feaae19126079d309ac8dd9a81372648437f1/contracts/base/BasePool.sol#L91

Tool used

Manual Review

Recommendation

Use call instead of transfer()