Missing access control on `FeeSplitter::setCurves` function. Malicious user can set his curves contract and withdraw all ethers from `FeeSplitter` using `FeeSplitter:claimFees` function.

[c4-bot-7]

Lines of code

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

Vulnerability details

Summary

FeeSplitter::setCurvesfunction does'nt have any access control. So anyone can call this and set curves contract to a malicious contract which gives the value in favour of malicious user. Some malicious curvesTokenSubject can buy 1st token in real Curves contract. He can set his own curves state variable in FeeSplitter.sol using setCurves function. Which will return curvesTokenSubject wanted unlimited balance output from curvesTokenBalance in FeeSplitter::balanceOf function. Based on that malicious balance curvesTokenSubject can claim all the fees present in FeeSplitter contract for all the holders but maliciouscurvesTokenSubject will withdraw all the ethers present in FeeSplitter and he will completely drain it.

Vulnerability Details and POC :

Clearly below we can see setCurves has no access control. So anyone can call it and set curves contract to his own malicious contract which will return the value in favour of malicious actor.

Vulnerable Code contracts/FeeSplitter.sol#L35-L37

      //@audit missing access control on Critical function
35:  function setCurves(Curves curves_) public {
36:        curves = curves_;
37:    }

Attack Path (POC) :

1. Attacker will buy 1 token from real Curves contract's buyCurvesToken function as he is first buyer and by passing his address in curvesTokenSubject. so it can call onBalanceChange and addFees functions in FeeSplitter. So that data.cumulativeFeePerToken have something. So it's difference with data.userFeeOffset[account] will not be 0 in updateFeeCredit function. So that it updates the data.unclaimedFees[account] for attacker address same as ether balance of FeeSplitter.

2. He will set his malicious curves contract in FeeSplitter::setCurves function. This contract implements a curvesTokenBalance and curvesTokenSupply function and returns the value according to the FeeSplitter total eth balance so he can claim all the ethers present in FeeSplitter.

3. Now he checks using getClaimableFees function how much balance should his malicious Curves curvesTokenBalance function return to make fees equal to FeeSplitter eth balance.

FeeSplitter::getClaimableFees

   function getClaimableFees(address token, address account) public view returns (uint256) {
       TokenData storage data = tokensData[token];
       uint256 balance = balanceOf(token, account);
       uint256 owed = (data.cumulativeFeePerToken - data.userFeeOffset[account]) * balance;
       return (owed / PRECISION) + data.unclaimedFees[account];
   }

4. Attacker set his curves to return that value when curvesTokenBalance called from balanceOf of FeeSplitter.

FeeSplitter::balanceOf

  function balanceOf(address token, address account) public view returns (uint256) {
     return curves.curvesTokenBalance(token, account) * PRECISION;
 }

1. Now he calls the claimFees function of FeeSplitter. In updateFeeCredit data.unclaimedFees[account] is updated based on calling balanceOf to get the attacker favorable balance and that balance is returned from malicious curves contract of attacker in balanceOf function. And claimable in claimFees function from getClaimableFees. So in the end claimable comes as full value of ether balance of FeeSplitter. And this fees balance is transferred to attacker account. And FeeSplitter is totally drained. FeeSplitter::claimFees

   function claimFees(address token) external {
       updateFeeCredit(token, msg.sender);
       uint256 claimable = getClaimableFees(token, msg.sender);
       if (claimable == 0) revert NoFeesToClaim();
       tokensData[token].unclaimedFees[msg.sender] = 0;
       payable(msg.sender).transfer(claimable);
       emit FeesClaimed(token, msg.sender, claimable);
   }
   

Impact

FeeSplitter contract which is holding fees or token holders will be drained. And No eth in this will be remained. No other token holder will get any fees. Attacker takes all tokens holders fees from FeeSPlitter.

Tools Used

Manual Review

Recommended Mitigation

Add access control to setCurves function. So that onlyOwner or some allowed roles can call this.

Note: Inherited Security modifiers is useless since it doesn't prevent anything. So I recommend to update that before using onlyOwner to this function.

Assessed type

Access Control

[c4-pre-sort]

raymondfam marked the issue as sufficient quality report

[c4-pre-sort]

raymondfam marked the issue as duplicate of #4

[c4-judge]

alcueca changed the severity to 2 (Med Risk)