Impact of ETH Transfer Handling Vulnerability in Contract

[c4-submissions]

Lines of code

https://github.com/code-423n4/2023-10-canto/blob/main/canto_ambient/contracts/mixins/LiquidityMining.sol#L286 https://github.com/code-423n4/2023-10-canto/blob/main/canto_ambient/contracts/mixins/LiquidityMining.sol#L193

Vulnerability details

Impact

The code provided attempts to send Ether (ETH) to an owner address using the call function without checking whether the address is a regular Ethereum address or a smart contract with a receive function. If the owner address is a contract without a receive function to handle ETH transfers, the operation will fail, and the ETH will become stuck in the contract's balance. This can result in an undesirable scenario where ETH sent to the contract cannot be retrieved or used as intended.

Proof of Concept

function claimAmbientRewards( address owner, bytes32 poolIdx, uint32[] memory weeksToClaim ) internal { .. if (rewardsToSend > 0) { (bool sent, ) = owner.call{value: rewardsToSend}(""); require(sent, "Sending rewards failed"); } }

function claimConcentratedRewards(
    address payable owner,
    bytes32 poolIdx,
    int24 lowerTick,
    int24 upperTick,
    uint32[] memory weeksToClaim
) internal {
    ..
    if (rewardsToSend > 0) {
        (bool sent, ) = owner.call{value: rewardsToSend}("");
        require(sent, "Sending rewards failed");
    }
}

Tools Used

Manual review

Recommended Mitigation Steps

Before attempting to send ETH to an address, check whether the address is a regular Ethereum address or a contract address. You can use the address type's .isContract() function to determine this.

Assessed type

call/delegatecall

[c4-pre-sort]

141345 marked the issue as duplicate of #64

[c4-pre-sort]

141345 marked the issue as sufficient quality report

[c4-judge]

dmvt marked the issue as not a duplicate

[c4-judge]

dmvt marked the issue as unsatisfactory: Overinflated severity

[dmvt]

User error and recommended mitigation doesn't work / is significantly worse.

[c4-judge]

dmvt changed the severity to QA (Quality Assurance)

[c4-judge]

dmvt marked the issue as grade-c