c4-submissions commented 1 year ago

Lines of code

https://github.com/code-423n4/2023-10-canto/blob/40edbe0c9558b478c84336aaad9b9626e5d99f34/canto_ambient/contracts/mixins/LiquidityMining.sol#L192-L196

Vulnerability details

Impact

The 'call' function is used to send ETH to the owner of the position.

The impact of the finding that the 'call' function used to send ETH to the owner of the position in line 192(https://github.com/code-423n4/2023-10-canto/blob/40edbe0c9558b478c84336aaad9b9626e5d99f34/canto_ambient/contracts/mixins/LiquidityMining.sol#L192) of LiquidityMining.sol. However, this approach can be problematic. If the owner is a malicious contract, they may have a fallback function that executes malicious code when receiving ETH, potentially leading to reentrancy exploits.

If an attacker is able to take control of the malicious contract, they can use the fallback function to execute a reentrancy attack and drain the contract of its ETH. This can result in significant financial losses for the contract and its users.

Furthermore, once the contract is part of a larger system, the impact of the attack can be even more severe and can result in the loss of user funds and damage to the reputation of the CANTO's community.

Proof of Concept

Suppose there is a malicious contract that has a fallback function that executes a reentrancy attack. The fallback function looks like this:

function() external payable {
    if (msg.sender == attacker) {
        // Reentrancy attack
        attacker.call{value: msg.value}("");
    }
}

The attacker contract has a call function that calls the fallback function of the malicious contract and triggers the reentrancy attack. The call function looks like this:

function callMaliciousContract(address maliciousContract) external payable {
    maliciousContract.call{value: msg.value}("");
}

Now suppose that the owner of the position in line 192-196:(https://github.com/code-423n4/2023-10-canto/blob/40edbe0c9558b478c84336aaad9b9626e5d99f34/canto_ambient/contracts/mixins/LiquidityMining.sol#L192-L196) of LiquidityMining.sol is the malicious contract. When the claimConcentratedLiquidityRewards function is called, it sends ETH directly to the malicious contract using the call function. This triggers the fallback function of the malicious contract, which executes the reentrancy attack and drains the contract of its ETH.

POC implementation at line #195:

// Map users' balances instead of sending ETH directly
mapping(address => uint256) public userRewards;

function claimConcentratedLiquidityRewards(address owner, uint256 poolIdx, int24 lowerTick, int24 upperTick, uint256[] calldata weeks) external {
    // ...
    // Calculate rewardsToSend
    // ...
    require(rewardsToSend > 0, "No rewards to claim");

    // Add rewards to the user's balance instead of sending ETH directly
    userRewards[owner] += rewardsToSend;

    // Update the rewards claimed mapping
    for (uint256 i = 0; i < weeks.length; i++) {
        uint256 week = weeks[i];
        if (weekOver(week) && !concLiquidityRewardsClaimed[owner][week]) {
            concLiquidityRewardsClaimed[owner][week] = true;
        }
    }
}

// Function to allow users to withdraw their rewards manually
function withdrawRewards() external {
    uint256 rewardsToSend = userRewards[msg.sender];
    require(rewardsToSend > 0, "No rewards to withdraw");

    // Set the user's balance to zero before transferring
    userRewards[msg.sender] = 0;

    // Perform the ETH transfer
    (bool sent, ) = msg.sender.call{value: rewardsToSend}("");
    require(sent, "Sending rewards failed");
}

Tools Used

Manual Review

Recommended Mitigation Steps

To mitigate this risk, it is recommended to use the "pull over push" pattern whenever possible. Instead of sending ETH directly to the owner, the protocol can keep a record of how many tokens each address is entitled to and allow users to manually withdraw their earnings. This prevents malicious contracts from executing code at the time of transfer and gives users more control over their funds.
One way to implement the "pull over push" pattern in this context would be to keep track of the rewards owed to each user in a mapping, and allow users to withdraw their rewards by calling a withdrawRewards function. Here's an example implementation:

mapping(address => uint256) private rewardsOwed;

function claimConcentratedLiquidityRewards(address owner, uint256 poolIdx, int24 lowerTick, int24 upperTick, uint256[] calldata weeks) external {
    // ...
    // Calculate rewardsToSend
    // ...
    rewardsOwed[owner] += rewardsToSend;
    // ...
}

function withdrawRewards() external {
    uint256 rewards = rewardsOwed[msg.sender];
    require(rewards > 0, "No rewards to withdraw");
    rewardsOwed[msg.sender] = 0;
    (bool success, ) = msg.sender.call{value: rewards}("");
    require(success, "Withdrawal failed");
}

In this implementation, the claimConcentratedLiquidityRewards function calculates the rewards owed to the owner of the position and adds them to the rewardsOwed mapping. The withdrawRewards function allows users to withdraw their rewards by transferring the ETH from the contract to the user's address using the call function. The rewardsOwed mapping is updated to reflect the fact that the user has withdrawn their rewards.

By using the "pull over push" pattern, this implementation reduces the risk of reentrancy exploits and gives users more control over their funds.

*It's important to note that the withdrawRewards function should only be called by the user who owns the balance, as it transfers ETH directly to the user's address. Additionally, you may want to consider adding additional security measures, such as a withdrawal limit or a withdrawal delay, to prevent potential attacks.

Recommended implementation

Can replace the if (rewardsToSend > 0) statement in line 192(https://github.com/code-423n4/2023-10-canto/blob/40edbe0c9558b478c84336aaad9b9626e5d99f34/canto_ambient/contracts/mixins/LiquidityMining.sol#L192) of the LiquidityMining.sol file with the following code:

// Add rewards to the user's balance instead of sending ETH directly
userRewards[owner] += rewardsToSend;

// Update the rewards claimed mapping
for (uint256 i = 0; i < weeks.length; i++) {
    uint256 week = weeks[i];
    if (weekOver(week) && !concLiquidityRewardsClaimed[owner][week]) {
        concLiquidityRewardsClaimed[owner][week] = true;
    }
}

This code adds the rewards owed to the owner of the position to the userRewards mapping instead of sending ETH directly. It also updates the concLiquidityRewardsClaimed mapping to indicate that the rewards have been claimed for the given position and week.

Note that will need to declare the userRewards and concLiquidityRewardsClaimed mappings at the top of your contract, like this:

mapping(address => uint256) public userRewards;
mapping(address => mapping(uint256 => bool)) public concLiquidityRewardsClaimed;

Additionally, we will need to implement the withdrawRewards function to allow users to withdraw their rewards manually. But can use the implementation I provided earlier for this purpose.

Assessed type

Reentrancy

c4-pre-sort commented 1 year ago

141345 marked the issue as duplicate of #54

c4-pre-sort commented 1 year ago

141345 marked the issue as sufficient quality report

c4-judge commented 1 year ago

dmvt marked the issue as unsatisfactory: Invalid

code-423n4 / 2023-10-canto-findings

The call function is used to send ETH to the owner of the position #102