Rigor Protocol contest - Extropy's results

File Project.sol

Check precision function

In the checkPrecision() function the natSpec comment reads:

/**
* @dev Check if precision is greater than 1000, if so, it reverts

However the internal pure function only returns true when the amount needed to be checked for precision is a multiple of 1000, or even when 0, i.e. must _amount muat be 0 or in 1000 increments.

* @param _amount amount needed to be checked for precision.
*/
function checkPrecision(uint256 _amount) internal pure {
    // Divide and multiply amount with 1000 should be equal to amount.
    // This ensures the amount is not too precise.
    require(
        ((_amount / 1000) * 1000) == _amount,
        "Project::Precision>=1000"
    );
}

For example, if _amount was 1999it would return false, because1999 / 1000 * 1000` equals 1000, and not 1999 (precision loss).

If _amount passed in parameter was 0, then 0 would be equal to _amount and the check would pass.

checkPrecision is called in:

• addTasks
• changeOrder

allowing to store a 0 cost for tasks in both functions, in changeOrder for example it modifies a task's price created in addTasks:

// Store new cost for the task
tasks[_taskID].cost = _newCost;

In changeOrder, it might trigger a revert in the following condition, if _newCost is 0, which is allowed, due to an underflow occurring by subtracting from 0.

else if (totalLent - _totalAllocated >= _newCost - _taskCost) {
    // Increase the difference of new cost and old cost to total allocated.
    totalAllocated += _newCost - _taskCost;
}

Recommendations

To minimize division errors, refactor your arithmetic to perform division last.

The output of an integer division operation is truncated to remove any non-zero remainder. Any arithmetic operations (other than addition and subtraction) that occur after a division operation will exacerbate this truncation error. To minimize this kind of error, refactor your arithmetic operations to perform division operations last. At a minimum, do not perform any multiplication or exponentiation after a division that may have non-zero truncation errors. Note however that ordering division last may increase the possibility of overflow. Example

In this example we compute (21/5)*7 in two different ways. Note that the true value is 29.4.

import "@openzeppelin/contracts/math/SafeMath.sol";

contract Test {
    using SafeMath for uint256;

    function bigError() public pure returns (uint256) {
        return uint256(21).div(5).mul(7); // Returns 28. Total error: 1.4.
    }

    function smallError() public pure returns (uint256) {
        return uint256(21).mul(7).div(5); // Returns 29. Total error: 0.4.
    }

}

The bigError() function performs the division first. The resulting intermediate value of uint256(21).div(5) is 4, which has a truncation error of 0.2 (because the real value of 21/5 is 4.2). This error increases 7-fold with the proceeding multiplication, yielding a total error of 1.4. The smallError() function performs the division last, and results in a smaller error. This is the recommended approach.

checkSignatureValidity function - delete functionality

Is the following line 891 in function checkSignatureValidity() intentional?

// delete from approvedHash
delete approvedHashes[_address][_hash];

From the natspec comments there is nothing indicating that the hash should be deleted, nor it is implied by the function name checkSignatureValidity.

/**
* @dev Internal function for checking signature validity
* @dev Checks if the signature is approved or recovered
* @dev Reverts if not

* @param _address address - address checked for validity
* @param _hash bytes32 - hash for which the signature is recovered

The internal functions checkSignature and checkSignatureTask (in turn calling checkSignatureValidity) can be called by the following external functions:

• inviteContractor
• updateProjectHash
• addTasks
• updateTaskHash
• updateTaskHash
• setComplete
• changeOrder

Whenever any of the above functions gets called (and if all required conditions are met) the hashes are deleted, these can be approved by anyone; but can they also be deleted by anyone via one of the external functions listed above?

/// @notice Returns mapping to keep track of all hashes (message or transaction) that have been approved by ANYONE
function approvedHashes(address \_signer, bytes32 \_hash)
external
view
returns (bool);

The full function is shown below, checkSignatureValidity. Notice the require statement _recoveredSignature == _address || approvedHashes[_address][_hash], it uses an || condition, not an &&; from this we can conclude that even if approvedHashes[_address][_hash] was used for access control reasons and deleted in order to avoid replay attacks, this condition would not even execute if the first part of || is true. Since the signature and hash can be retrieved from transaction data, it can be considered public information.

function checkSignatureValidity(
    address _address,
    bytes32 _hash,
    bytes memory _signature,
    uint256 _signatureIndex
) internal {
    address _recoveredSignature = SignatureDecoder.recoverKey(
        _hash,
        _signature,
        _signatureIndex
    );
    require(
        _recoveredSignature == _address || approvedHashes[_address][_hash],
        "Project::invalid signature"
    );
    // delete from approvedHash
    delete approvedHashes[_address][_hash];
}

The above function is in turn called by another internal function:

function checkSignature(bytes calldata _data, bytes calldata _signature)
    internal
{
    // Calculate hash from bytes
    bytes32 _hash = keccak256(_data);

    // When there is no contractor
    if (contractor == address(0)) {
        // Check for builder's signature
        checkSignatureValidity(builder, _hash, _signature, 0);
    }
    // When there is a contractor
    else {
        // When builder has delegated his rights to contractor
        if (contractorDelegated) {
            //  Check contractor's signature
            checkSignatureValidity(contractor, _hash, _signature, 0);
        }
        // When builder has not delegated rights to contractor
        else {
            // Check for both B and GC signatures
            checkSignatureValidity(builder, _hash, _signature, 0);
            checkSignatureValidity(contractor, _hash, _signature, 1);
        }
    }
}

builder and contractor are public addresses, therefore the security of this function relies on the secrecy of the original bytes calldata _data, which would require an attacker to find a hash collision using an order with different parameters:

// Calculate hash from bytes
bytes32 _hash = keccak256(_data);

The same line is implemented in the checkSignatureTask function.

function checkSignatureTask(
    bytes calldata _data,
    bytes calldata _signature,
    uint256 _taskID
) internal {
    // Calculate hash from bytes
    bytes32 _hash = keccak256(_data);
    ...

Conventional MitM attacks such as XSS or clickjacking can be used in order to steal raw transaction bytes data.

File: Community.sol

Recommendation : Change storage to memory

line 184

line 230

line 648

line 738

Recommendation : Change code order

Line can be moved before line 184

Recommendation : Remove unnecessary variable

_community.memberCount can be used directly in lines 198 and 199

Recommendation : Remove repeated require block

This syntax gets used four times so it could be a modifier in all instances except in the escrow():

require(
_msgSender() == _communities[_communityID].owner,
"Community::!Owner"
);

Likewise on line 386it is not capitalised unlike the others

"Community::!owner"

Recommendation : Remove unecessary return declaration

type name of sender not necessary line 903

Recommendation : Remove unecessary asignment

Can be set by default to true instea of doing so explicitely

File : HomeFi.sol

Recommendation : Shorten shorten ipfs (CID v1) hash to 32 bytes line 284

mintNFT function, parameter string memory _tokenURI can be shortened to create a bytes32 type, rather than a string that stores 128 bytes for IPFS CIDs version 0.