Fei Protocol contest - 0x0x0x's results

Handle

0x0x0x

Vulnerability details

Concept

When requery is called, token1OutBase can get updated. token1OutBase is a crucial parameter, which directly tells how many tokens will the user get.

If a user creates a transaction and before getting included in a block, token1OutBase gets reduced. Users will swap for an unexpected ratio and lose funds..

For swap operations in most contracts (like sushi), there is an input parameter for minimum outputted tokens. This ensures that the user can make swap requests with deterministic results. This protects users against such problems.

As seen in https://github.com/fei-protocol/fei-protocol-core/blob/cc14a601d60e6d550bc88ec84da0999fa8a3daf0/contracts/oracle/collateralization/CollateralizationOracleWrapper.sol#L128, minProtocolEquity can be set by governor or admin arbitrarily (indirectly also token1OutBase). On Oracle CachedValueUpdate event is emitted, but there is also no guarantee, if someone calls requery with new values or not. So users can easily get frontrunned or swap with an unexpected ratio.

Users don't have enough control when calling swap operation ngmi. There is also no protection for users against unexpected behaviour. I consider this a high-risk issue.

Mitigation steps

I strongly recommend adding events, view functions and a minimum output parameter for ngmi. Also, it would be nice to have a view function for token1OutBase and token0InBase. Minimum output parameter is utilized on almost all popular contracts with similar functionality, since it is crucial for user safety.

Handle

0x0x0x

Vulnerability details

Concept

merkleRoot is defined with 0 value and then assigned in constructor. The best approach for having a such parameter is using immutable keyword. Also operations using merkleRoot will consume less gas.

Handle

0x0x0x

Vulnerability details

Concept

Applying == true doesn't make any difference and consume extra gas. Since this operation can be seen as an identity function.

Scope

./PegExchanger.sol:38:        require(isEnabled() == true, "Proposals are not both passed");
./PegExchanger.sol:111:            isEnabled() == true,
./TRIBERagequit.sol:65:        require(isEnabled() == true, "Proposals are not both passed");
./TRIBERagequit.sol:69:            verifyClaim(thisSender, key, merkleProof) == true,

Handle

0x0x0x

Vulnerability details

Concept

PegExchanger.sol#takeFrom, PegExchanger.sol#giveTo and TRIBERagequit.sol#takeFrom are implemented in a similar manner. After transferFrom call, the return value is cached to a parameter called check, after that it is controlled whether check is true. But the used transferFrom functions never return false. They simply revert if there is a problem with the transaction. Therefore, it is possible to implement those functions more gas efficient.

Therefore following implementation:

function takeFrom(address target, uint256 amount) internal {
        bool check = token0.transferFrom(target, address(this), amount);
        require(check, "erc20 transfer failed");
    }

Can be replaced with:

function takeFrom(address target, uint256 amount) internal {
        token0.transferFrom(target, address(this), amount);
    }

Handle

0x0x0x

Vulnerability details

Concept

When a variable is declared solidity assigns the default value. In case the contract assigns the value again, it costs extra gas.

Example:uint x = 0 costs more gas than uint x without having any different functionality.

Scope

./PegExchanger.sol:22:    uint256 public expirationBlock = 0;
./TRIBERagequit.sol:20:    uint256 public expirationBlock = 0;
./TRIBERagequit.sol:26:    uint256 public token1OutBase = 0;
./PegExchanger.sol:14:    bool public party0Accepted = false; // rgt timelock accepted
./PegExchanger.sol:15:    bool public party1Accepted = false; // tribe timelock accepted
./TRIBERagequit.sol:22:    bool public party0Accepted = false; // rgt timelock accepted
./TRIBERagequit.sol:23:    bool public party1Accepted = false; // tribe timelock accepted
./TRIBERagequit.sol:40:    bool public init = false;

Handle

0x0x0x

Vulnerability details

Concept

The returned value is not used. Both functions update global variables and there is no need for a return.

Handle

0x0x0x

Vulnerability details

Concept

External functions usually consume less gas compared to public functions.

Occurences

In PegExchange:

exchange
party0Accept
party1Accept
setExpirationBlock

In TRIBERagequit:

ngmi
requery
party0Accept
party1Accept

Handle

0x0x0x

Vulnerability details

Proof of Concept

Example:

for (uint i = 0; i < arr.length; i++) {

//Operations not effecting the length of the array.

}

Loading length of array costs gas. Therefore, the length should be cached, if the length of the array doesn't change inside the loop. Furthermore, there is no need to assign the initial value 0. This costs extra gas.

Recommended implementation:

uint length = arr.length;

for (uint i; i < length; i++) {

//Operations not effecting the length of the array.

}

By doing so the length is only loaded once rather than loading it as many times as iterations (Therefore, less gas is spent).

Occurences

./TRIBERagequit.sol:165: for (uint256 i = 0; i < proof.length; i++) {