Fei Protocol contest - WatchPug's results

Handle

WatchPug

Vulnerability details

https://github.com/code-423n4/2021-11-fei/blob/add34324513b863f58e4ef7b3cd0c12d776dbb7f/contracts/PegExchanger.sol#L39-L39

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

exchange() can not be called from the contract itself, therefore require(msg.sender != address(this) is redundant.

Handle

WatchPug

Vulnerability details

https://github.com/code-423n4/2021-11-fei/blob/add34324513b863f58e4ef7b3cd0c12d776dbb7f/contracts/PegExchanger.sol#L68-L71

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

In PegExchanger.sol#giveTo(), transferFrom is used to transfer funds from address(this) to the target. Since transferFrom includes unnecessary check of if (spender != src && spenderAllowance != uint96(-1)) {..}, it's more gas efficient to use transfer instead.

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-fei/blob/add34324513b863f58e4ef7b3cd0c12d776dbb7f/contracts/TRIBERagequit.sol#L205-L208

require(
            msg.sender == party0Timelock,
            "Only the timelock for party 0 may call this function"
        );

https://github.com/code-423n4/2021-11-fei/blob/add34324513b863f58e4ef7b3cd0c12d776dbb7f/contracts/TRIBERagequit.sol#L217-L220

require(
            msg.sender == party1Timelock,
            "Only the timelock for party 1 may call this function"
        );

Handle

WatchPug

Vulnerability details

https://github.com/code-423n4/2021-11-fei/blob/add34324513b863f58e4ef7b3cd0c12d776dbb7f/contracts/TRIBERagequit.sol#L110-L131

function requery() public returns (int256) {
    (
        uint256 _pcvValue, //  pcv value
        uint256 _userFei, // user fei
        int256 newProtocolEquity,
        bool validity
    ) = oracle.pcvStats();
    if (minProtocolEquity == 0) {
        if (init == false) {
            minProtocolEquity = newProtocolEquity;
            recalculate();
            init = true;
            return minProtocolEquity;
        }
    }
    if (minProtocolEquity > newProtocolEquity) {
        minProtocolEquity = newProtocolEquity;
        recalculate();
        return minProtocolEquity;
    }
    return minProtocolEquity;
}

Change to else if can save gas:

function requery() public returns (int256) {
    (
        uint256 _pcvValue, //  pcv value
        uint256 _userFei, // user fei
        int256 newProtocolEquity,
        bool validity
    ) = oracle.pcvStats();
    if (minProtocolEquity == 0) {
        if (init == false) {
            minProtocolEquity = newProtocolEquity;
            recalculate();
            init = true;
            return minProtocolEquity;
        }
    } else if (minProtocolEquity > newProtocolEquity) {
        minProtocolEquity = newProtocolEquity;
        recalculate();
        return minProtocolEquity;
    }
    return minProtocolEquity;
}

Handle

WatchPug

Vulnerability details

Reading array length at each iteration of the loop takes 6 gas (3 for mload and 3 to place memory_offset) in the stack.

Caching the array length in the stack saves around 3 gas per iteration.

Instances include:

• TRIBERagequit.sol#processProof()

  https://github.com/code-423n4/2021-11-fei/blob/add34324513b863f58e4ef7b3cd0c12d776dbb7f/contracts/TRIBERagequit.sol#L165-L165

Handle

WatchPug

Vulnerability details

https://github.com/code-423n4/2021-11-fei/blob/add34324513b863f58e4ef7b3cd0c12d776dbb7f/contracts/TRIBERagequit.sol#L28-L28

uint256 public preMergeCirculatingTribe = 711206739862133000000000000;

Some storage variables include preMergeCirculatingTribe will not never be changed and they should not be.

Changing them to constant can save gas.

Handle

WatchPug

Vulnerability details

Redundant code increase contract size and gas usage at deployment.

Can be changed to require(isEnabled(), "...").

https://github.com/code-423n4/2021-11-fei/blob/add34324513b863f58e4ef7b3cd0c12d776dbb7f/contracts/PegExchanger.sol#L110-L113

require(
    isEnabled() == true,
    "Contract must be enabled before admin functions called"
);

Other exmaples include:

https://github.com/code-423n4/2021-11-fei/blob/add34324513b863f58e4ef7b3cd0c12d776dbb7f/contracts/TRIBERagequit.sol#L59-L71

function ngmi(
    uint256 multiplier,
    uint256 key,
    bytes32[] memory merkleProof
) public {
    require(isExpired() == false, "Redemption period is over");
    require(isEnabled() == true, "Proposals are not both passed");
    require(minProtocolEquity > 0, "no equity");
    address thisSender = msg.sender;
    require(
        verifyClaim(thisSender, key, merkleProof) == true,
        "invalid proof"
    );

https://github.com/code-423n4/2021-11-fei/blob/add34324513b863f58e4ef7b3cd0c12d776dbb7f/contracts/PegExchanger.sol#L36-L45

function exchange(uint256 multiplier) public {
    require(isExpired() == false, "Redemption period is over");
    require(isEnabled() == true, "Proposals are not both passed");
    require(msg.sender != address(this), "????");
    uint256 token0TakenTotal = token0InBase * multiplier;
    uint256 token1GivenTotal = token1OutBase * multiplier;
    takeFrom(msg.sender, token0TakenTotal);
    giveTo(msg.sender, token1GivenTotal);
    emit Exchange(msg.sender, token0TakenTotal, token1GivenTotal);
}

Handle

WatchPug

Vulnerability details

For the arithmetic operations that will never over/underflow, using the unchecked directive (Solidity v0.8 has default overflow/underflow checks) can save some gas from the unnecessary internal over/underflow checks.

For example:

https://github.com/code-423n4/2021-11-fei/blob/add34324513b863f58e4ef7b3cd0c12d776dbb7f/contracts/TRIBERagequit.sol#L227-L231

function _startCountdown() internal {
    if (expirationBlock == 0) {
        expirationBlock = block.number + 6400 * 3; // approx. 3 days in blocks
    }
}

block.number + 6400 * 3 will never overflow.

Handle

WatchPug

Vulnerability details

https://github.com/code-423n4/2021-11-fei/blob/add34324513b863f58e4ef7b3cd0c12d776dbb7f/contracts/PegExchanger.sol#L22-L22

uint256 public expirationBlock = 0;

Setting uint256 variables to 0 is redundant as they default to 0.

Other examples include:

https://github.com/code-423n4/2021-11-fei/blob/add34324513b863f58e4ef7b3cd0c12d776dbb7f/contracts/TRIBERagequit.sol#L20-L20

https://github.com/code-423n4/2021-11-fei/blob/add34324513b863f58e4ef7b3cd0c12d776dbb7f/contracts/TRIBERagequit.sol#L26-L26

https://github.com/code-423n4/2021-11-fei/blob/add34324513b863f58e4ef7b3cd0c12d776dbb7f/contracts/TRIBERagequit.sol#L31-L31

Handle

WatchPug

Vulnerability details

https://github.com/code-423n4/2021-11-fei/blob/add34324513b863f58e4ef7b3cd0c12d776dbb7f/contracts/TRIBERagequit.sol#L96-L106

function recalculate() public returns (uint256) {
    if (minProtocolEquity > 0) {
        token0InBase = scalar;
        token1OutBase =
            (scalar * uint256(minProtocolEquity)) /
            preMergeCirculatingTribe;
    } else {
        token1OutBase = 0;
    }
    return token1OutBase;
}

Given that everytime minProtocolEquity got updated, recalculate() will be called internaly by requery(), there is no need to make recalculate() a public method.

Calling recalculate() from the outside will be a waste of gas.

Handle

WatchPug

Vulnerability details

Per to the doc:

1. Mints more tribe
2. Send tribe to the PegExchanger

The tribe tokens used by PegExchanger.sol for exchange are sent from the tribe gov directly to the wallet balance of the PegExchanger contract.

It most certainly will leftover some of the unused tribe tokens, especially when isExpired(). There is no way for the admin or anyone else to retrieve these leftover tribe tokens.

PoC

1. The tribe gov mint and send 1M tribe tokens to the PegExchanger;
2. PegExchanger only used 80k of them before the ExpirationBlock;
3. 20k of tribe tokens will be frozen in the contract.

Recommendation

Consider changing to: mint the tribe tokens to the tribe gov or a special proposed contract that holds the tribe tokens, and approve a certain amount to the PegExchange contract.

Change the giveTo function to:

https://github.com/code-423n4/2021-11-fei/blob/add34324513b863f58e4ef7b3cd0c12d776dbb7f/contracts/PegExchanger.sol#L68-L71

function giveTo(address target, uint256 amount) internal {
    bool check = token1.transferFrom(TRIBE_GOV, target, amount);
    require(check, "erc20 transfer failed");
}

When the exchange ends, the allowance can be revoked.

Handle

WatchPug

Vulnerability details

Here are some examples that the code style does not follow the best practices:

https://github.com/code-423n4/2021-11-fei/blob/add34324513b863f58e4ef7b3cd0c12d776dbb7f/contracts/PegExchanger.sol#L57-L71

/// @notice function which ultimately transfers the RGT
/// @param target the address of which to transfer RGT from
/// @param amount the amount of RGT to transfer from the target address
function takeFrom(address target, uint256 amount) internal {
    bool check = token0.transferFrom(target, address(this), amount);
    require(check, "erc20 transfer failed");
}

/// @notice function which ultimately transfers the TRIBE
/// @param target the address of which to transfer TRIBE to
/// @param amount the amount of TRIBE to transfer to the target address
function giveTo(address target, uint256 amount) internal {
    bool check = token1.transferFrom(address(this), target, amount);
    require(check, "erc20 transfer failed");
}

Handle

WatchPug

Vulnerability details

Using ++i is more gas efficient than i++, especially in a loop.

We suggest to use unchecked {++i} to even better gas performance. (for >= 0.8)

For example:

https://github.com/code-423n4/2021-11-fei/blob/add34324513b863f58e4ef7b3cd0c12d776dbb7f/contracts/TRIBERagequit.sol#L165-L176

for (uint256 i = 0; i < proof.length; i++) {
    bytes32 proofElement = proof[i];
    if (computedHash <= proofElement) {
        computedHash = keccak256(
            abi.encodePacked(computedHash, proofElement)
        );
    } else {
        computedHash = keccak256(
            abi.encodePacked(proofElement, computedHash)
        );
    }
}

Handle

WatchPug

Vulnerability details

https://github.com/code-423n4/2021-11-fei/blob/add34324513b863f58e4ef7b3cd0c12d776dbb7f/contracts/TRIBERagequit.sol#L229-L229

expirationBlock = block.number + 6400 * 3; // approx. 3 days in blocks

Recommendation

Change to:

expirationBlock = block.number + 19200; // approx. 3 days in blocks

Handle

WatchPug

Vulnerability details

https://github.com/code-423n4/2021-11-fei/blob/add34324513b863f58e4ef7b3cd0c12d776dbb7f/contracts/TRIBERagequit.sol#L138-L145

function verifyClaim(
    address claimer,
    uint256 key,
    bytes32[] memory merkleProof
) private view returns (bool) {
    bytes32 leaf = keccak256(abi.encodePacked(claimer, key));
    return verifyProof(merkleProof, merkleRoot, leaf);
}

leaf is unnecessary as it's being used only once. Can be changed to:

function verifyClaim(
    address claimer,
    uint256 key,
    bytes32[] memory merkleProof
) private view returns (bool) {
    return verifyProof(merkleProof, merkleRoot, keccak256(abi.encodePacked(claimer, key)));
}

https://github.com/code-423n4/2021-11-fei/blob/add34324513b863f58e4ef7b3cd0c12d776dbb7f/contracts/TRIBERagequit.sol#L59-L82

function ngmi(
    uint256 multiplier,
    uint256 key,
    bytes32[] memory merkleProof
) public {
    require(isExpired() == false, "Redemption period is over");
    require(isEnabled() == true, "Proposals are not both passed");
    require(minProtocolEquity > 0, "no equity");
    address thisSender = msg.sender;
    require(
        verifyClaim(thisSender, key, merkleProof) == true,
        "invalid proof"
    );
    require(
        (claimed[thisSender] + multiplier) <= key,
        "already ragequit all you tokens"
    );
    claimed[thisSender] = claimed[thisSender] + multiplier;
    uint256 token0TakenTotal = token0InBase * multiplier;
    uint256 token1GivenTotal = token1OutBase * multiplier;
    takeFrom(thisSender, token0TakenTotal);
    giveTo(thisSender, token1GivenTotal);
    emit Exchange(thisSender, token0TakenTotal, token1GivenTotal);
}

thisSender is unnecessary, it can be replaced with msg.sender.

Handle

WatchPug

Vulnerability details

https://github.com/code-423n4/2021-11-fei/blob/add34324513b863f58e4ef7b3cd0c12d776dbb7f/contracts/TRIBERagequit.sol#L25-L25

uint256 public token0InBase = 2**256 - 1;

Recommendation

Change to:

uint256 public token0InBase = type(uint256).max;

Handle

WatchPug

Vulnerability details

For the storage variables that will be accessed multiple times, 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-fei/blob/add34324513b863f58e4ef7b3cd0c12d776dbb7f/contracts/TRIBERagequit.sol#L110-L131

function requery() public returns (int256) {
    (
        uint256 _pcvValue, //  pcv value
        uint256 _userFei, // user fei
        int256 newProtocolEquity,
        bool validity
    ) = oracle.pcvStats();
    if (minProtocolEquity == 0) {
        if (init == false) {
            minProtocolEquity = newProtocolEquity;
            recalculate();
            init = true;
            return minProtocolEquity;
        }
    }
    if (minProtocolEquity > newProtocolEquity) {
        minProtocolEquity = newProtocolEquity;
        recalculate();
        return minProtocolEquity;
    }
    return minProtocolEquity;
}

minProtocolEquity can be cached.

Recommendation

Change to:

function requery() public returns (int256) {
    (
        uint256 _pcvValue, //  pcv value
        uint256 _userFei, // user fei
        int256 newProtocolEquity,
        bool validity
    ) = oracle.pcvStats();
    uint256 _minProtocolEquity = minProtocolEquity;
    if (_minProtocolEquity == 0) {
        if (init == false) {
            minProtocolEquity = newProtocolEquity;
            recalculate();
            init = true;
            return minProtocolEquity;
        }
    }
    if (_minProtocolEquity > newProtocolEquity) {
        minProtocolEquity = newProtocolEquity;
        recalculate();
        return minProtocolEquity;
    }
    return _minProtocolEquity;
}

Handle

WatchPug

Vulnerability details

https://github.com/code-423n4/2021-11-fei/blob/add34324513b863f58e4ef7b3cd0c12d776dbb7f/contracts/TRIBERagequit.sol#L96-L106

function recalculate() public returns (uint256) {
    if (minProtocolEquity > 0) {
        token0InBase = scalar;
        token1OutBase =
            (scalar * uint256(minProtocolEquity)) /
            preMergeCirculatingTribe;
    } else {
        token1OutBase = 0;
    }
    return token1OutBase;
}

The returns of recalculate() are unused, so that they can be removed to avoid unnecessary storage read of token1OutBase and save some gas.

Recommendation

Change to:

function recalculate() public {
    if (minProtocolEquity > 0) {
        token0InBase = scalar;
        token1OutBase =
            (scalar * uint256(minProtocolEquity)) /
            preMergeCirculatingTribe;
    } else {
        token1OutBase = 0;
    }
}

Handle

WatchPug

Vulnerability details

The current implementation requires a minter role of the FEI token:

https://github.com/code-423n4/2021-11-fei/blob/add34324513b863f58e4ef7b3cd0c12d776dbb7f/contracts/TRIBERagequit.sol#L185-L187

function giveTo(address target, uint256 amount) internal {
    token1.mint(target, amount);
}

We believe it's unnecessary and can potentially pose a security risk. For example, if merkleRoot is somehow being malformed or tempered. The TribeRagequit can mint a much bigger amount of FEI token than expected and cause systematic risk affecting the whole FEI protocol.

Otoh, if TribeRagequit is going to be deployed as an upgradable contract, this change can also help prevent the deployer of TribeRagequit from abusing the minter role.

Recommendation

Change from:

• Set the TribeRagequit as a minter

To:

• Mint a predetermined amount of FEI tokens to the gov contract or a special purpose holdings contract;
• Approve a predetermined amount of allowance to the TribeRagequit contract;
• Burn the unused FEI, and revoke the allowance when TribeRagequit.isExpired().

In addition to the changes above, a new function to burn the unused FEI tokens when TribeRagequit.isExpired() can be added.

Handle

WatchPug

Vulnerability details

https://github.com/code-423n4/2021-11-fei/blob/add34324513b863f58e4ef7b3cd0c12d776dbb7f/contracts/PegExchanger.sol#L36-L45

function exchange(uint256 multiplier) public {
    require(isExpired() == false, "Redemption period is over");
    require(isEnabled() == true, "Proposals are not both passed");
    require(msg.sender != address(this), "????");
    uint256 token0TakenTotal = token0InBase * multiplier;
    uint256 token1GivenTotal = token1OutBase * multiplier;
    takeFrom(msg.sender, token0TakenTotal);
    giveTo(msg.sender, token1GivenTotal);
    emit Exchange(msg.sender, token0TakenTotal, token1GivenTotal);
}

https://github.com/code-423n4/2021-11-fei/blob/add34324513b863f58e4ef7b3cd0c12d776dbb7f/contracts/PegExchanger.sol#L60-L63

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

https://github.com/code-423n4/2021-11-fei/blob/add34324513b863f58e4ef7b3cd0c12d776dbb7f/contracts/PegExchanger.sol#L68-L71

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

takeFrom() and giveTo() are unnecessary as they are used only once. Therefore they can be inlined in exchange() to make the code simpler and save gas.

Recommendation

Change to:

function exchange(uint256 multiplier) public {
    require(isExpired() == false, "Redemption period is over");
    require(isEnabled() == true, "Proposals are not both passed");
    require(msg.sender != address(this), "????");
    uint256 token0TakenTotal = token0InBase * multiplier;
    uint256 token1GivenTotal = token1OutBase * multiplier;
    require(token0.transferFrom(msg.sender, address(this), token0TakenTotal), "erc20 transfer failed");
    require(token1.transfer(msg.sender, token1GivenTotal), "erc20 transfer failed");
    emit Exchange(msg.sender, token0TakenTotal, token1GivenTotal);
}