Fei Protocol contest - Czar102's results

Handle

Czar102

Vulnerability details

Impact

Unneeded check in line 39 in PegExchanger:

require(msg.sender != address(this), "????");

msg.sender can't call any functions other than specified in the documentation.

From empirical data, deleting this statement saves magically 46,654 gas for deployment!

Tools Used

truffle, ganache

Recommended Mitigation Steps

Delete this require(...) statement.

Handle

Czar102

Vulnerability details

Impact

The variable preMergeCirculatingTribe is not marked constant, thus every read is very gas-inefficient. The deployment costs 16,014 gas more.

Proof of Concept

code

Tools Used

slither, truffle, ganache

Recommended Mitigation Steps

Mark the variable constant.

Handle

Czar102

Vulnerability details

Impact

Boolean variables should not be compared with other booleans.

Proof of Concept

PegExchanger.exchange(uint256) (contracts/PegExchanger.sol#36-45) compares to a boolean constant:
        -require(bool,string)(isExpired() == false,Redemption period is over) (contracts/PegExchanger.sol#37)
PegExchanger.exchange(uint256) (contracts/PegExchanger.sol#36-45) compares to a boolean constant:
        -require(bool,string)(isEnabled() == true,Proposals are not both passed) (contracts/PegExchanger.sol#38)
PegExchanger.setExpirationBlock(uint256) (contracts/PegExchanger.sol#101-115) compares to a boolean constant:
        -require(bool,string)(isEnabled() == true,Contract must be enabled before admin functions called) (contracts/PegExchanger.sol#110-113)
TribeRagequit.ngmi(uint256,uint256,bytes32[]) (contracts/TRIBERagequit.sol#59-82) compares to a boolean constant:
        -require(bool,string)(isExpired() == false,Redemption period is over) (contracts/TRIBERagequit.sol#64)
TribeRagequit.ngmi(uint256,uint256,bytes32[]) (contracts/TRIBERagequit.sol#59-82) compares to a boolean constant:
        -require(bool,string)(isEnabled() == true,Proposals are not both passed) (contracts/TRIBERagequit.sol#65)
TribeRagequit.ngmi(uint256,uint256,bytes32[]) (contracts/TRIBERagequit.sol#59-82) compares to a boolean constant:
        -require(bool,string)(verifyClaim(thisSender,key,merkleProof) == true,invalid proof) (contracts/TRIBERagequit.sol#68-71)
TribeRagequit.requery() (contracts/TRIBERagequit.sol#110-131) compares to a boolean constant:
        -init == false (contracts/TRIBERagequit.sol#118)
Reference: https://github.com/crytic/slither/wiki/Detector-Documentation#boolean-equality

Tools Used

slither

Recommended Mitigation Steps

Delete " == true" and instead of " == false" - negate the variable.

Handle

Czar102

Vulnerability details

Impact

Three variables in requery() function return data are declared, but never used. This makes the deployment cost 3,679 gas more (calculated empirically).

Proof of Concept

code

Tools Used

truffle, ganache

Recommended Mitigation Steps

Do not assign these return values to variables.

Handle

Czar102

Vulnerability details

Impact

Functions exchange(uint256), setExpirationBlock(uint256), ngmi(uint256,uint256,bytes32[]), requery(), party0Accept(), party1Accept() (the last two in both contracts) are marked public, while can be external. This uses more gas.

Proof of Concept

PegExchanger.sol#36-45

PegExchanger.sol#80-86

PegExchanger.sol#89-95

PegExchanger.sol#101-115

TRIBERagequit.sol#59-82

TRIBERagequit.sol#110-131

TRIBERagequit.sol#204-213

TRIBERagequit.sol#216-225

Tools Used

slither

Recommended Mitigation Steps

Mark these functions external.

Handle

Czar102

Vulnerability details

Impact

Usage of local variables (swaps, dups on stack) uses more gas than CALLER opcode. Also, code is more clear when using msg.sender instead of a local variable.

Proof of Concept

link

Tools Used

Manual analysis

Recommended Mitigation Steps

Use msg.sender instead of a local variable thisSender.

Handle

Czar102

Vulnerability details

Impact

Function giveTo() in PegExchanger uses transferFrom(...), which reads the allowance of the contract, despite it is not needed in this case. This theoretically uses more than 2000 additional gas compared to the transfer() function.

Recommended Mitigation Steps

Consider using transfer() method instead of transferFrom(...).