Using `.call` for external calls leaves the contract vulnerable to reentrancy.

[c4-submissions]

Lines of code

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

Vulnerability details

Impact

claimConcentratedRewards and claimAmbientRewards call external contracts via call. This could leave the contract vulnerable to reentrancy attacks if the recipient contract calls back into the contract before updating state.

Proof of Concept

The calls to external contracts occur on these lines:

claimConcentratedRewards

Line 193: 
(bool sent, ) = owner.call{value: rewardsToSend}("");

claimAmbientRewards

Line 281:
(bool sent, ) = owner.call{value: rewardsToSend}("");  

The issue is that .call is used to send Ether to the owner's address. This makes the contract vulnerable to reentrancy.

For example, if the owner's contract has a fallback function that calls back into claimRewards again before the state is updated, it could continually drain funds.

Here is an in-depth explanation of the reentrancy vulnerability introduced by using .call in claimConcentratedRewards and claimAmbientRewards:

You can see that these functions send Ether to user addresses after calculating rewards:

function claimConcentratedRewards() {

  // Calculate rewards
  uint256 rewards = 100;

  address payable user = 0x1234...;

  // Send rewards
  user.call{value: rewards}("");

}

The problem is call forwards all available gas to the callee and returns any data or error values from the call. This allows the user's contract to execute code and call back into the original contract while claimConcentratedRewards is still executing.

For example, the user could have a fallback function that calls claimConcentratedRewards again:

contract Attack {

  function() external payable {
    LiquidityMining(msg.sender).claimConcentratedRewards();  
  }

}

So, here is the full execution flow would be:

1. LiquidityMining calls claimConcentratedRewards
2. Calculates rewards as 100 ETH
3. Sends 100 ETH to Attack contract via call
4. Attack fallback function is triggered
5. Attack calls LiquidityMining.claimConcentratedRewards again
6. More rewards are calculated and sent to Attack
7. Repeat 2-6 until all funds drained

The rewards state was never updated in LiquidityMining, so Attack can continuously drain funds.

Tools Used

Vs

Recommended Mitigation Steps

Using call{value: amount}("") prevents reentrancy but is still unsafe as it provides no gas stipend and could fail if the recipient is a contract.

A better solution is to use the transfer function which is safe against reentrancy:

owner.transfer(rewardsToSend);

Or send Ether in a separate transaction after updating state:

// Update state 

(bool sent, ) = owner.send(rewardsToSend);
require(sent, "Transfer failed");

This separates the external call from the state changes, preventing reentrancy.

The key is to avoid calling external contracts via call when in the middle of updating state. Transferring funds separately after state changes is a safer pattern.

Assessed type

Reentrancy

[c4-pre-sort]

141345 marked the issue as low quality report

[c4-judge]

dmvt marked the issue as unsatisfactory: Insufficient quality

[c4-judge]

dmvt marked the issue as unsatisfactory: Insufficient quality