FairSide contest - WatchPug's results

Handle

WatchPug

Vulnerability details

Every reason string takes at least 32 bytes.

Use short reason strings that fits in 32 bytes or it will become more expensive.

Instances include:

https://github.com/code-423n4/2021-11-fairside/blob/20c68793f48ee2678508b9d3a1bae917c007b712/contracts/token/FSD.sol#L299-L307

require(
    governanceThreshold <=
        getPriorConvictionScore(
            msg.sender,
            governanceTributes[num].blockNumber
        ) &&
        isGovernance[msg.sender],
    "FSD::claimGovernanceTribute: Not a governance member"
);

https://github.com/code-423n4/2021-11-fairside/blob/20c68793f48ee2678508b9d3a1bae917c007b712/contracts/token/FSD.sol#L314-L317

require(
    currentPhase != Phase.Final,
    "FSD::phaseAdvance: FSD is already at its final phase"
);

Handle

WatchPug

Vulnerability details

The check if _wallets.length <= 2 is redundant as the length of _wallets parameter must be 2.

https://github.com/code-423n4/2021-11-fairside/blob/20c68793f48ee2678508b9d3a1bae917c007b712/contracts/network/FSDNetwork.sol#L520-L533

function setMembershipWallets(address[2] calldata _wallets) external {
    //todo internal
    require(
        membership[msg.sender].wallets[0] == address(0) &&
            membership[msg.sender].wallets[1] == address(0),
        "FSDNetwork::setMembershipWallets: Cannot have more than three wallets per membership"
    );
    require(
        _wallets.length <= 2,
        "FSDNetwork::setMembershipWallets: Too many wallets"
    );
    membership[msg.sender].wallets = _wallets;
}

Recommendation

Remove the redundant check.

Handle

WatchPug

Vulnerability details

https://github.com/code-423n4/2021-11-fairside/blob/20c68793f48ee2678508b9d3a1bae917c007b712/contracts/network/FSDNetwork.sol#L482-L495

function setAssessors(address[] calldata _assessors) external {
        require(
            msg.sender == GOVERNANCE_ADDRESS,
            "FSDNetwork::setAssessors: Insufficient Privileges"
        );

        uint256 assessorsLength = _assessors.length;
        require(
            assessorsLength == 3,
            "FSDNetwork::setAssessors: Number of assessors must be three"
        );

        assessors = _assessors;
    }

Recommendation

Change to:

function setAssessors(address[3] calldata _assessors) external {
        require(
            msg.sender == GOVERNANCE_ADDRESS,
            "FSDNetwork::setAssessors: Insufficient Privileges"
        );

        assessors = _assessors;
    }

Handle

WatchPug

Vulnerability details

https://github.com/code-423n4/2021-11-fairside/blob/20c68793f48ee2678508b9d3a1bae917c007b712/contracts/network/FSDNetwork.sol#L310-L315

// 20% as staking rewards
fsd.safeTransfer(address(fsd), stakingRewards);
fsd.addRegistrationTribute(stakingRewards);

// 7.5% towards governance
fsd.safeTransfer(address(fsd), governancePoolRewards);

The 2 transfers to the same address can be done in one external call to save gas

Recommendation

Change to:

// 20% as staking rewards
// 7.5% towards governance
fsd.safeTransfer(address(fsd), governancePoolRewards + stakingRewards);
fsd.addRegistrationTribute(stakingRewards);

Handle

WatchPug

Vulnerability details

The checks in the for loop can be changed to else if to save gas and make sure msg.sender != sigAssessor.

https://github.com/code-423n4/2021-11-fairside/blob/20c68793f48ee2678508b9d3a1bae917c007b712/contracts/network/FSDNetwork.sol#L616-L649

function _isApprovedByAssessors(
        bytes memory sig,
        uint256 id,
        Action action
    ) private view returns (bool) {
        bytes32 digest = _hashTypedDataV4(
            keccak256(abi.encode(CSR_ACTION, id, action))
        );

        address sigAssessor = ECDSA.recover(digest, sig);
        uint256 assessorsLength = assessors.length;
        bool assessorOne;
        bool assessorTwo;

        for (uint256 i = 0; i < assessorsLength; i++) {
            if (msg.sender == assessors[i]) {
                assessorOne = true;
            }
            if (sigAssessor == assessors[i]) {
                assessorTwo = true;
            }
        }

        require(
            assessorOne && assessorTwo,
            "FSDNetwork::_isApprovedByAssessors: Not an Assessor"
        );
        require(
            msg.sender != sigAssessor,
            "FSDNetwork::_isApprovedByAssessors: Cannot be the single Assessor"
        );

        return true;
    }

Recommendation

Change to:

function _isApprovedByAssessors(
        bytes memory sig,
        uint256 id,
        Action action
    ) private view returns (bool) {
        bytes32 digest = _hashTypedDataV4(
            keccak256(abi.encode(CSR_ACTION, id, action))
        );

        address sigAssessor = ECDSA.recover(digest, sig);
        uint256 assessorsLength = assessors.length;
        bool assessorOne;
        bool assessorTwo;

        for (uint256 i = 0; i < assessorsLength; i++) {
            if (msg.sender == assessors[i]) {
                assessorOne = true;
            } else if (sigAssessor == assessors[i]) {
                assessorTwo = true;
            }
        }

        require(
            assessorOne && assessorTwo,
            "FSDNetwork::_isApprovedByAssessors: Not an Assessor"
        );

        return true;
    }

Handle

WatchPug

Vulnerability details

For the storage variables that will be accessed multiple times, especially in for loops, cache and read from the stack can save ~100 gas from each extra read (SLOAD after Berlin).

For example:

https://github.com/code-423n4/2021-11-fairside/blob/20c68793f48ee2678508b9d3a1bae917c007b712/contracts/dependencies/TributeAccrual.sol#L77-L88

    function totalAvailableTribute(uint256 offset)
        external
        view
        override
        returns (uint256 total)
    {
        for (uint256 i = offset; i < totalTributes; i++)
            total = total.add(availableTribute(i));

        for (uint256 i = offset; i < totalGovernanceTributes; i++)
            total = total.add(availableGovernanceTribute(i));
    }

totalTributes and totalGovernanceTributes can be cached.