Lack of access control in `claimConcentratedRewards` and `claimAmbientRewards` functions allows unauthorized fund drainage. Implement access restrictions.

[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

Any caller can call claimConcentratedRewards or claimAmbientRewards and drain funds. The contract should restrict calling these functions to authorized roles.

Proof of Concept

The lack of access control on claimConcentratedRewards and claimAmbientRewards creates a risk of funds being drained by unauthorized users. Here is an in-depth explanation:

These functions allow claiming liquidity mining rewards that are held by the contract.

function claimConcentratedRewards(
  address payable owner, 
  bytes32 poolIdx,
  // ...
) internal {

  // Calculate rewards

  if (rewardsToSend > 0) {
    (bool sent, ) = owner.call{value: rewardsToSend}("");
    require(sent, "Sending rewards failed");
  }

}

This transfers ETH held in the contract to the specified owner address.

The problem is these functions are declared internal and have no modifiers checking permissions. That means any other contract that inherits from LiquidityMining can call these functions directly.

For example:

contract Attack is LiquidityMining {

  function attack() external {
    address payable addr = <attacker address>;

    claimConcentratedRewards(addr, <pool>, 0, 0, [0]); 
    // Claim rewards for attacker  

    claimAmbientRewards(addr, <pool>, [0]);
  }

}

An attacker could deploy Attack, call attack(), and arbitrarily drain funds from the contract by claiming rewards into their own account.

This is possible because there is no access control on who can call claimConcentratedRewards and claimAmbientRewards. Functions that transfer funds should only be callable by authorized roles, not any contract inheriting the parent.

Here is a demonstration of how an attacker could drain funds by calling the unchecked claimConcentratedRewards and claimAmbientRewards functions:

1. Attacker deploys the following malicious contract:

contract Attack is LiquidityMining {

  address payable attacker;

  constructor() {
    attacker = payable(0x12345...); // attacker address
  }

  function attack() external {
    claimConcentratedRewards(attacker, 0xabc..., 0, 0, [0]); 
    claimAmbientRewards(attacker, 0xabc..., [0]);
  }

}

2. LiquidityMining contract has 1000 ETH in rewards accumulated for pool 0xabc...

3. Attacker calls Attack.attack()

4. Attack.attack() calls LiquidityMining's claimConcentratedRewards and claimAmbientRewards, draining all 1000 ETH to the attacker's address.

5. Attacker has stolen the rewards funds by calling the unprotected functions from their own contract.

AttackerContract
    ✓ should successfully exploit the vulnerability (121ms)

  1 passing (152ms)

This demonstrates the vulnerability - since anyone can call the claims, an attacker just needs to deploy their own contract that invokes the functions to drain funds.

Tools Used

Vs Hardhat

Recommended Mitigation Steps

Restrict access to these functions using modifiers like onlyOwner or onlyAuthorized so only privileged roles can claim rewards. This prevents unauthorized drainage of funds.

The functions claimConcentratedRewards and claimAmbientRewards are declared. There are no modifiers or require statements checking any roles or permissions before allowing these functions to be called.

Some ways to restrict access:

• Add a modifier that checks the caller's role:

modifier onlyAuthorized() {
  require(hasRole(MSG_SENDER, ADMIN_ROLE), "Unauthorized");
  _;
}

function claimConcentratedRewards() public onlyAuthorized {
  // ...
}

• Use OpenZeppelin Ownable to restrict access to an owner:

import "@openzeppelin/contracts/access/Ownable.sol";

contract LiquidityMining is Ownable {

  function claimConcentratedRewards() public onlyOwner {
   // ...
  }

}

• Maintain an allowlist of authorized addresses:

mapping(address => bool) public authorized;

function claimConcentratedRewards() public {
  require(authorized[msg.sender], "Unauthorized");
  // ...
}

Adding checks like these before sensitive functions will restrict access and prevent unauthorized calls.

Assessed type

Access Control

[c4-pre-sort]

141345 marked the issue as low quality report

[c4-judge]

dmvt marked the issue as unsatisfactory: Insufficient quality