Uninitialized protocol fee address could cause loss of funds

[c4-submissions]

Lines of code

https://github.com/code-423n4/2023-09-asymmetry/blob/main/contracts/AfEth.sol#L277

Vulnerability details

Summary

The address to which fees are sent is not initialized in the AfEth contract, and could cause loss of funds if fees are collected before this address is properly configured.

Impact

AfEth protocol fees are collected from rewards coming from the Votium strategy. After tokens are claimed, the depositRewards() will take a portion of it and send them to the configured feeAddress.

https://github.com/code-423n4/2023-09-asymmetry/blob/main/contracts/AfEth.sol#L272-L279

272:     function depositRewards(uint256 _amount) public payable {
273:         IVotiumStrategy votiumStrategy = IVotiumStrategy(vEthAddress);
274:         uint256 feeAmount = (_amount * protocolFee) / 1e18;
275:         if (feeAmount > 0) {
276:             // solhint-disable-next-line
277:             (bool sent, ) = feeAddress.call{value: feeAmount}("");
278:             if (!sent) revert FailedToSend();
279:         }

The main issue here is that this address is not part of the contract initialization. If left uninitialized, the implementation will send the collected ETH from fees to the address(0).

Recommendation

Configure the feeAddress in the contract's initializer to make sure this address is correctly configured when the instance is set up.

- function initialize() external initializer {
+ function initialize(address _feeAddress) external initializer {
      _transferOwnership(msg.sender);
      ratio = 5e17;
+     feeAddress = _feeAddress;
  }

Assessed type

ETH-Transfer

[c4-judge]

0xleastwood marked the issue as primary issue

[c4-judge]

0xleastwood marked the issue as selected for report

[c4-sponsor]

elmutt (sponsor) confirmed

[c4-sponsor]

elmutt marked the issue as disagree with severity

[c4-sponsor]

elmutt (sponsor) acknowledged

[c4-judge]

0xleastwood marked the issue as not selected for report

[0xleastwood]

Not sure if I agree with the severity here. Let's say setFeeAddress() is called during contract deployment, can anything really be done by front-running this and calling depositRewards()? It doesn't seem to be the case. Downgrading to QA.

[c4-judge]

0xleastwood changed the severity to QA (Quality Assurance)