FairSide contest - 0x0x0x's results

Handle

0x0x0x

Vulnerability details

Impact

Proof of Concept

The amount of ethereum at funding pool is simply checked by pool.balance. This is not recommended, because the amount of total funding can change by withdrawing funds. In case, funds are withdrawn before the end of hatch phase, it can result in getting funded more than claimed on launch. In current implementation, funds shouldn't be touched till the hatch phase finished, otherwise this behaviour can damage the reputation of this project.

Recommended Mitigation Steps

Accumulate total funding with a variable rather than checking balance

Mismatch between whipaper and code

I checked FSD.sol and the whitepaper https://fairside-network.gitbook.io/fairside-network/white-paper/augmented-bonding-curve. According to whitepaper, during hatch phase, funding pool should be funded till 500 ETH, but on code it is 2000 ETH.

Tools Used

Manual

Handle

0x0x0x

Vulnerability details

Impact

Gas optimization

Proof of Concept

The function first checks _reserveDelta < 0 for calculations and after that we apply the same check for return statement. Return statements can directly written inside if-else statement. Furthermore, it will produce more readable code.

Tools Used

Manual

Recommended Mitigation Steps

Current code:

        if (_reserveDelta < 0) {
            uint256 capitalPostWithdrawal = capitalPool.sub(
                _abs(_reserveDelta)
            );
            require(
                capitalPostWithdrawal >= fShare,
                "ABC::_calculateDeltaOfFSD: Insufficient Capital to Withdraw"
            );
            nextSupply = FairSideFormula.g(capitalPostWithdrawal, fShare);
        } else {
            nextSupply = FairSideFormula.g(
                capitalPool.add(uint256(_reserveDelta)),
                fShare
            );
        }

        return
            _reserveDelta < 0
                ? currentSupply.sub(nextSupply)
                : nextSupply.sub(currentSupply);

Recommended code:

       if (_reserveDelta < 0) {
           uint256 capitalPostWithdrawal = capitalPool.sub(
               _abs(_reserveDelta)
           );
           require(
               capitalPostWithdrawal >= fShare,
               "ABC::_calculateDeltaOfFSD: Insufficient Capital to Withdraw"
           );
           nextSupply = FairSideFormula.g(capitalPostWithdrawal, fShare);
           return currentSupply.sub(nextSupply);
       } else {
           nextSupply = FairSideFormula.g(
               capitalPool.add(uint256(_reserveDelta)),
               fShare
           );
           return nextSupply.sub(currentSupply);
       }

Handle

0x0x0x

Vulnerability details

Impact

Gas optimization

Proof of Concept

When forceOnchain, we can ensure the same logic by using less require statements. Since when its forceOnchain, we check whether the length is 1, we do not need the require statements before it. We can have those checks in an else part to save gas.

The code block L396-L418:

        require(
            targets.length != 0,
            "FairSideDAO::propose: must provide actions"
        );
        require(
            targets.length <= proposalMaxOperations(),
            "FairSideDAO::propose: too many actions"
        );

        if (forceOnchain) {
            require(
                targets.length == 1,
                "FairSideDAO::propose: only a single action is permitted for a forced onchain vote"
            );
            bytes32 signatureHash = keccak256(abi.encode(signatures[0]));
            require(
                (targets[0] == address(this) &&
                    signatureHash == DISABLE_OFFCHAIN_HASH) ||
                    (targets[0] == address(FSD) &&
                        signatureHash == ADJUST_THRESHOLD_HASH),
                "FairSideDAO::propose: only adjustment of governance threshold or revocation of offchain process can be forced onchain"
            );
        }

Can be replaced with:

        if (forceOnchain) {
            require(
                targets.length == 1,
                "FairSideDAO::propose: only a single action is permitted for a forced onchain vote"
            );
            bytes32 signatureHash = keccak256(abi.encode(signatures[0]));
            require(
                (target## Recommended Mitigation Stepss[0] == address(this) &&
                    signatureHash == DISABLE_OFFCHAIN_HASH) ||
                    (targets[0] == address(FSD) &&
                        signatureHash == ADJUST_THRESHOLD_HASH),
                "FairSideDAO::propose: only adjustment of governance threshold or revocation of offchain process can be forced onchain"
            );
        }
        else {
            require(
                targets.length != 0,
                "FairSideDAO::propose: must provide actions"
            );
            require(
                targets.length <= proposalMaxOperations(),
                "FairSideDAO::propose: too many actions"
            );
        }

Tools Used

Manual analysis

Handle

0x0x0x

Vulnerability details

Finding

!= 0 costs less gass compared to > 0 for unsigned integer

Occurences

contracts/dao/FairSideDAO.sol:306:            proposalCount >= proposalId && proposalId > 0,
contracts/dependencies/ERC20ConvictionScore.sol:129:                upper = center > 0 ? center - 1 : 0;
contracts/dependencies/ERC20ConvictionScore.sol:176:        if (locked > 0) {
contracts/dependencies/ERC20ConvictionScore.sol:280:        if (checkpointCount > 0) {
contracts/dependencies/ERC20ConvictionScore.sol:308:        if (nCheckpoints > 0 && checkpoint.fromBlock == blockNumber) {
contracts/dependencies/Withdrawable.sol:58:        require(reserveAmount > 0, "FSD::withdraw: Insufficient Withdrawal");
contracts/network/FSDNetwork.sol:230:            costShareBenefit % 10 ether == 0 && costShareBenefit > 0,
contracts/token/FSD.sol:174:        require(bonded > 0, "FSD::mintHatch: Insufficient Deposit");
contracts/token/FSDVesting.sol:127:            tokenClaim > 0,

Recommended Mitigation Steps

Change > 0 with != 0.

Handle

0x0x0x

Vulnerability details

Impact

Gas optimization

Finding

contracts/token/FSD.sol:174: require(bonded > 0, "FSD::mintHatch: Insufficient Deposit");

On L170, we check whether bonded is bigger than 5 ETH. After multiplying with HATCH_CURVE_RATIO, it is still over 0. Therefore, it is a tautology and not needed.

Proof of Concept

Provide direct links to all referenced code in GitHub. Add screenshots, logs, or any other relevant proof that illustrates the concept.

Tools Used

Manual analysis