Missing validation and update function in NapierPool.sol risks losing of all pool fees

Links

https://github.com/sherlock-audit/2024-01-napier/blob/6313f34110b0d12677b389f0ecb3197038211e12/v1-pool/src/NapierPool.sol#L107-L140 https://github.com/sherlock-audit/2024-01-napier/blob/6313f34110b0d12677b389f0ecb3197038211e12/v1-pool/src/NapierPool.sol#L526-L534 https://github.com/sherlock-audit/2024-01-napier/blob/6313f34110b0d12677b389f0ecb3197038211e12/v1-pool/src/PoolFactory.sol#L50-L92 https://github.com/sherlock-audit/2024-01-napier/blob/6313f34110b0d12677b389f0ecb3197038211e12/v1-pool/src/NapierPool.sol#L544-L556

Summary

Protocol fees denominated in both underlying and baseLpt; retrievable only by the feeReceipient can get locked in the contract.

Vulnerability Detail

There is no zero address validation of the feeRecipient address passed in deploying a new pool; neither in PoolFactory::deploy() nor in NapierPool::constructor(). The only way to withdraw fees is calling NapierPool::skim() which sends fees to feeRecipient.

function skim() external nonReentrant {
    (uint256 _totalUnderlying, uint256 _totalBaseLpt) = (totalUnderlying,totalBaseLpt);
    uint256 baseLptExcess = _balance(tricrypto) - _totalBaseLpt;
    uint256 feesAndExcess = _balance(underlying) - _totalUnderlying;
    if (baseLptExcess != 0) tricrypto.safeTransfer(feeRecipient, baseLptExcess);
    if (feesAndExcess != 0) underlying.safeTransfer(feeRecipient, feesAndExcess);
}

There is also no way to update the feeRecipient in setFeeParameter() if any mistake is made or change required although this functionality exists In tranche.sol with setFeeRecipient(); which seems to be missing from NapierPool.sol

function setFeeParameter(bytes32 paramName, uint256 value) external {
    if (factory.owner() != msg.sender) revert Errors.PoolOnlyOwner();

    if (paramName == "lnFeeRateRoot") {
        if (value > MAX_LN_FEE_RATE_ROOT) revert ErrorsLnFeeRateRootTooHigh();
        lnFeeRateRoot = uint80(value); // unsafe cast here is Okaybecause we checked the value is less than MAX_LN_FEE_RATE_ROOT
    } else if (paramName == "protocolFeePercent") {
        if (value > MAX_PROTOCOL_FEE_PERCENT) revert ErrorsProtocolFeePercentTooHigh();
        protocolFeePercent = uint8(value); // unsafe cast here is Okay
    } else {
        revert Errors.PoolInvalidParamName();
    }
}

Impact

Accumulated pool fees will get irretrievably lost in this contract if the wrong value for feeRecipient is passed in.

Code Snippet

Tool used

Foundry Testing Manual Review

Recommendation

Add zero address check to NapierPool::constructor() and update setFeeParameter() to allow updating of feeRecipient

-    function setFeeParameter(bytes32 paramName, uint256 value) external {
+    function setFeeParameter(bytes32 paramName, bytes calldata value) external {
        if (factory.owner() != msg.sender) revert Errors.PoolOnlyOwner();

        if (paramName == "lnFeeRateRoot") {
-           if (value > MAX_LN_FEE_RATE_ROOT) revert Errors.LnFeeRateRootTooHigh();
-           lnFeeRateRoot = uint80(value); // unsafe cast here is Okay because we checked the value is less than MAX_LN_FEE_RATE_ROOT
+            uint256 lnValue = abi.decode(value, (uint256));
+           if (lnValue > MAX_LN_FEE_RATE_ROOT) revert Errors.LnFeeRateRootTooHigh();
+           lnFeeRateRoot = uint80(lnValue);
        } else if (paramName == "protocolFeePercent") {
-           if (value > MAX_PROTOCOL_FEE_PERCENT) revert Errors.ProtocolFeePercentTooHigh();
-           protocolFeePercent = uint8(value); // unsafe cast here is Okay
+           uint256 protocolValue = abi.decode(value, (uint256));
+           if (protocolValue > MAX_PROTOCOL_FEE_PERCENT) revert Errors. ProtocolFeePercentTooHigh();
+           protocolFeePercent = uint8(protocolValue);
+       } else if (paramName == "feeRecipient") {
+           address feeRecipientAddress = abi.decode(value, (address));
+           if (feeRecipientAddress == address(0)) revert Errors.FeeRecipientCannotBeZero();
+           feeRecipient = feeRecipientAddress;
        } else {
            revert Errors.PoolInvalidParamName();
        }
    }

sherlock-audit / 2024-01-napier-judging

Falconhoof - Missing validation and update function in NapierPool.sol risks losing of all pool fees #123