Rigor Protocol contest - codexploder's results

Lines of code

https://github.com/code-423n4/2022-08-rigor/blob/b17b2a11d04289f9e927c71703b42771dd7b86a4/contracts/HomeFi.sol#L185

Vulnerability details

Impact

There is no higher cap limit on lenderFee which allows Admin to set this to abrupt high values. The impact will be seen at lendToProject , where most of funds for project will move to lender fee

Proof of Concept

1. Observe the replaceLenderFee function

function replaceLenderFee(uint256 _newLenderFee)
        external
        override
        onlyAdmin
    {
        // Revert if no change in lender fee
        require(lenderFee != _newLenderFee, "HomeFi::!Change");

        // Reset variables
        lenderFee = _newLenderFee;

        emit LenderFeeReplaced(_newLenderFee);
    }

2. As we can see that "lenderFee " is directly assigned the passed in value of "_newLenderFee" without any max value check

3. This directly impacts lendToProject as _amountToProject will be calculated after deducting _lenderFee. This _lenderFee will move to treasury which is controlled by Admin

function lendToProject(
        uint256 _communityID,
        address _project,
        uint256 _lendingAmount,
        bytes calldata _hash
    )
        external
        virtual
        override
        nonReentrant
        whenNotPaused
        isPublishedToCommunity(_communityID, _project)
    {

....
uint256 _lenderFee = (_lendingAmount * _projectInstance.lenderFee()) /
            (_projectInstance.lenderFee() + 1000);

        // Calculate amount going to project. Lending amount - lending fee.
        uint256 _amountToProject = _lendingAmount - _lenderFee;
....
}

Recommended Mitigation Steps

Keep a max cap for lenderFee which will prevent any abrupt value being set by Admin

Invalid signer are also accepted

Contract: Disputes

Issue: In case signature provided is invalid, SignatureDecoder.recoverKey will return with 0 address. The function still process this 0 address and creates a new dispute with 0 address owner

Recommendation: Kindly add below check:

function raiseDispute(bytes calldata _data, bytes calldata _signature)
        external
        override
        onlyProject
    {
        // Recover signer from signature
        address _signer = SignatureDecoder.recoverKey(
            keccak256(_data),
            _signature,
            0
        );
require(_signer != address(0), "Incorrect signature");
}

initialize can be called by any external user

Contract: Disputes

Issue: It seems that user can call the initialize function before owner has chance to do the same. This will initialize the homeFi variable incorrectly. Although this is not serious as Owner can immediately discard this contract but would be good to restrict this function

Recommendation: Kindly allow only owner/deployer to call this function