Frax Ether Liquid Staking contest - 0x1f8b's results

Lines of code

https://github.com/code-423n4/2022-09-frax/blob/55ea6b1ef3857a277e2f47d42029bc0f3d6f9173/src/frxETHMinter.sol#L200

Vulnerability details

Impact

frxETHMinter contract is incompatible with major tokens such as USDT, BNB or OMG. The recoverERC20 method could not work as expected, and some tokens can be lost.

The following problems are found:

• The transfer is performed without checking the result, so it could return false and result in a loss of funds.
• Is not used the safeX methods in order to avoid the boolean result when it is not defined, for example in USDT.

Proof of Concept

Some tokens do not return a bool (e.g. USDT, BNB, OMG) on ERC20 methods, (e.g. BNB) may return a bool for some methods, but fail to do so for others. This resulted in stuck BNB tokens in Uniswap v1 (details).

Reference:

• EIP-20

NOTES: The following specifications use syntax from Solidity 0.4.17 (or above). Callers MUST handle false from returns (bool success). Callers MUST NOT assume that false is never returned!

This is the code of USDT:

• https://etherscan.io/token/0xdac17f958d2ee523a2206206994597c13d831ec7#code

contract ERC20Basic {
uint public _totalSupply;
function totalSupply() public constant returns (uint);
function balanceOf(address who) public constant returns (uint);
+ function transfer(address to, uint value) public;
event Transfer(address indexed from, address indexed to, uint value);
}

contract ERC20 is ERC20Basic {
function allowance(address owner, address spender) public constant returns (uint);
+ function transferFrom(address from, address to, uint value) public;
+ function approve(address spender, uint value) public;
event Approval(address indexed owner, address indexed spender, uint value);
}

As you can see, it lacks the return defined in the ERC20 standard. So calling approve, transfer or transferFrom methods directly will check for a return that will cause the transaction to fail.

Here you have a list with tokens (outdated) that don't return a boolean in transfer:

• https://gist.githubusercontent.com/lukas-berlin/f587086f139df93d22987049f3d8ebd2/raw/1f937dc8eb1d6018da59881cbc633e01c0286fb0/Tokens%20missing%20return%20values%20in%20transfer

Recommended Mitigation Steps

• For transfers use safeTransfer and safeTransferFrom.

• Low
  • 1. Not compatible with some tokens like USDT
  • 2. Mixing and Outdated compiler
  • 3. Lack of checks
  • 4. Minters can burn any amount of tokens from an arbitrary address
  • 5. Minters can mint any amount of tokens to an arbitrary address
  • 6. Useless time lock
• Non critical
  • 7. Gap in minters_array

Low

1. Not compatible with some tokens like USDT

frxETHMinter contract is incompatible with major tokens such as USDT, that it's because when performing an 'approve', it is required to do an approve(0) first, to avoid some front-running token protections.

Affected source code:

• frxETHMinter.sol:75

2. Mixing and Outdated compiler

The pragma version used are:

pragma solidity ^0.8.0;
pragma solidity >=0.8.0;

Note that mixing pragma is not recommended. Because different compiler versions have different meanings and behaviors, it also significantly raises maintenance costs. As a result, depending on the compiler version selected for any given file, deployed contracts may have security issues.

The minimum required version must be 0.8.16; otherwise, contracts will be affected by the following important bug fixes:

0.8.3:

• Optimizer: Fix bug on incorrect caching of Keccak-256 hashes.

0.8.4:

• ABI Decoder V2: For two-dimensional arrays and specially crafted data in memory, the result of abi.decode can depend on data elsewhere in memory. Calldata decoding is not affected.

0.8.9:

• Immutables: Properly perform sign extension on signed immutables.
• User Defined Value Type: Fix storage layout of user defined value types for underlying types shorter than 32 bytes.

0.8.13:

• Code Generator: Correctly encode literals used in abi.encodeCall in place of fixed bytes arguments.

0.8.14:

• ABI Encoder: When ABI-encoding values from calldata that contain nested arrays, correctly validate the nested array length against calldatasize() in all cases.
• Override Checker: Allow changing data location for parameters only when overriding external functions.

0.8.15

• Code Generation: Avoid writing dirty bytes to storage when copying bytes arrays.
• Yul Optimizer: Keep all memory side-effects of inline assembly blocks.

0.8.16

• Code Generation: Fix data corruption that affected ABI-encoding of calldata values represented by tuples: structs at any nesting level; argument lists of external functions, events and errors; return value lists of external functions. The 32 leading bytes of the first dynamically-encoded value in the tuple would get zeroed when the last component contained a statically-encoded array.

Apart from these, there are several minor bug fixes and improvements.

3. Lack of checks

The following methods have a lack of checks if the received argument is an address, it's good practice in order to reduce human error to check that the address specified in the constructor or initialize is different than address(0).

Affected source code for address(0):

• In ERC20PermitPermissionedMint.sol:34 is not checked but it was in setTimelock
• In OperatorRegistry.sol:41 is not checked but it was in setTimelock
• frxETHMinter.sol:53-55

4. Minters can burn any amount of tokens from an arbitrary address

miters role can burn any amount of tokens from an arbitrary address.

This is needless and poses a severe risk of centralization. A malicious or compromised owner can take advantage of this.

Affected source code:

• ERC20PermitPermissionedMint.sol:54

5. Minters can mint any amount of tokens to an arbitrary address

The minters can mint an arbitrary amount of tokens to an arbitrary address.

This is needless and poses a severe risk of centralization. A malicious or compromised owner can take advantage of this.

Affected source code:

• ERC20PermitPermissionedMint.sol:54

6. Useless time lock

The advantages provided by the timelock in terms of decentralization disappear by having an owner who can manage the contract in the same way, so the contract is centralized to the actions of the owner

Affected source code:

• ERC20PermitPermissionedMint.sol:41
• OperatorRegistry.sol:46

---

Non critical

7. Gap in minters_array

When an entry from the ERC20PermitPermissionedMint.minters_array array is deleted, the last element of the array is not placed in the deleted position and the size of the array is reduced, so there is a gap that is not necessary and can cause problems in dApps.

Affected source code:

• ERC20PermitPermissionedMint.sol:86

• Gas
  • 1. Don't use the length of an array for loops condition
  • 2. There's no need to set default values for variables
    • Total gas saved: 8 * 7 = 56
  • 3. Reduce boolean comparison
    • Total gas saved: 18 * 2 + 15 * 1 = 51
  • 4. Avoid compound assignment operator in state variables
    • Total gas saved: 13 * 4 = 52
  • 5. Use calldata instead of memory
  • 6. ++i costs less gas compared to i++ or i += 1
  • 7. Reduce the size of error messages Long revert Strings
    • Total gas saved: 18 * 1 = 18
  • 8. Use Custom Errors instead of Revert Strings to save Gas
    • Total gas saved: 9 * 22 = 198
  • 9. Optimize xERC4626.totalAssets
  • 10. Change bool to uint256 can save gas
  • 11. Argument not needed

Gas

1. Don't use the length of an array for loops condition

It's cheaper to store the length of the array inside a local variable and iterate over it.

Affected source code:

• ERC20PermitPermissionedMint.sol:84
• OperatorRegistry.sol:114

2. There's no need to set default values for variables

If a variable is not set/initialized, the default value is assumed (0, false, 0x0 ... depending on the data type). You are simply wasting gas if you directly initialize it with its default value.

Proof of concept (without optimizations):

pragma solidity 0.8.15;

contract TesterA {
function testInit() public view returns (uint) { uint a = 0; return a; }
}

contract TesterB {
function testNoInit() public view returns (uint) { uint a; return a; }
}

Gas saving executing: 8 per entry

TesterA.testInit:   21392
TesterB.testNoInit: 21384   

Affected source code:

• ERC20PermitPermissionedMint.sol:84
• OperatorRegistry.sol:63
• OperatorRegistry.sol:84
• OperatorRegistry.sol:114
• frxETHMinter.sol:63
• frxETHMinter.sol:64
• frxETHMinter.sol:94

Total gas saved: 8 * 7 = 56

3. Reduce boolean comparison

Compare a boolean value using == true or == false instead of using the boolean value is more expensive.

NOT opcode it's cheaper than using EQUAL or NOTEQUAL when the value it's false, or just the value without == true when it's true, because it will use less opcodes inside the VM.

Proof of concept (without optimizations):

pragma solidity 0.8.16;

contract TesterA {
function testEqual(bool a) public view returns (bool) { return a == true; }
}

contract TesterB {
function testNot(bool a) public view returns (bool) { return a; }
}

Gas saving executing: 18 per entry for == true

TesterA.testEqual:   21814
TesterB.testNot:     21796   

pragma solidity 0.8.16;

contract TesterA {
function testEqual(bool a) public view returns (bool) { return a == false; }
}

contract TesterB {
function testNot(bool a) public view returns (bool) { return !a; }
}

Gas saving executing: 15 per entry for == false

TesterA.testEqual:   21814
TesterB.testNot:     21799

Affected source code:

Use the value instead of == true:

• ERC20PermitPermissionedMint.sol:46
• ERC20PermitPermissionedMint.sol:78

Use the value instead of == false:

• ERC20PermitPermissionedMint.sol:68

Total gas saved: (18 * 2) + (15 * 1) = 51

4. Avoid compound assignment operator in state variables

Using compound assignment operators for state variables (like State += X or State -= X ...) it's more expensive than using operator assignment (like State = State + X or State = State - X ...).

Proof of concept (without optimizations):

pragma solidity 0.8.15;

contract TesterA {
uint private _a;
function testShort() public {
_a += 1;
}
}

contract TesterB {
uint private _a;
function testLong() public {
_a = _a + 1;
}
}

Gas saving executing: 13 per entry

TesterA.testShort: 43507
TesterB.testLong:  43494

Affected source code:

• xERC4626.sol:67
• xERC4626.sol:72
• frxETHMinter.sol:97
• frxETHMinter.sol:168

Total gas saved: 13 * 4 = 52

5. Use calldata instead of memory

Some methods are declared as external but the arguments are defined as memory instead of as calldata.

By marking the function as external it is possible to use calldata in the arguments shown below and save significant gas.

Recommended change:

-   function setWithdrawalCredential(bytes memory _new_withdrawal_pubkey) external onlyByOwnGov {
+   function setWithdrawalCredential(bytes calldata _new_withdrawal_pubkey) external onlyByOwnGov {
        require(numValidators() == 0, "Clear validator array first");
        curr_withdrawal_pubkey = _new_withdrawal_pubkey;
        emit WithdrawalCredentialSet(_new_withdrawal_pubkey);
    }

Affected source code:

• OperatorRegistry.sol:181-186

6. ++i costs less gas compared to i++ or i += 1

++i costs less gas compared to i++ or i += 1 for unsigned integers, as pre-increment is cheaper (about 5 gas per iteration). This statement is true even with the optimizer enabled.

i++ increments i and returns the initial value of i. Which means:

uint i = 1;
i++; // == 1 but i == 2

But ++i returns the actual incremented value:

uint i = 1;
++i; // == 2 and i == 2 too, so no need for a temporary variable

In the first case, the compiler has to create a temporary variable (when used) for returning 1 instead of 2 I suggest using ++i instead of i++ to increment the value of an uint variable. Same thing for --i and i--

Keep in mind that this change can only be made when we are not interested in the value returned by the operation, since the result is different, you only have to apply it when you only want to increase a counter.

Affected source code:

• ERC20PermitPermissionedMint.sol:84

7. Reduce the size of error messages (Long revert Strings)

Shortening revert strings to fit in 32 bytes will decrease deployment time gas and will decrease runtime gas when the revert condition is met.

Revert strings that are longer than 32 bytes require at least one additional mstore, along with additional overhead for computing memory offset, etc.

Proof of concept (without optimizations):

pragma solidity 0.8.15;

contract TesterA {
function testShortRevert(bool path) public view {
require(path, "test error");
}
}

contract TesterB {
function testLongRevert(bool path) public view {
require(path, "test big error message, more than 32 bytes");
}
}

Gas saving executing: 18 per entry

TesterA.testShortRevert: 21886   
TesterB.testLongRevert:  21904       

Affected source code:

• frxETHMinter.sol:167

Total gas saved: 18 * 1 = 18

8. Use Custom Errors instead of Revert Strings to save Gas

Custom errors from Solidity 0.8.4 are cheaper than revert strings (cheaper deployment cost and runtime cost when the revert condition is met)

Source Custom Errors in Solidity:

Starting from Solidity v0.8.4, there is a convenient and gas-efficient way to explain to users why an operation failed through the use of custom errors. Until now, you could already use strings to give more information about failures (e.g., revert("Insufficient funds.");), but they are rather expensive, especially when it comes to deploy cost, and it is difficult to use dynamic information in them.

Custom errors are defined using the error statement, which can be used inside and outside of contracts (including interfaces and libraries).

Proof of concept (without optimizations):

pragma solidity 0.8.15;

contract TesterA {
function testRevert(bool path) public view {
 require(path, "test error");
}
}

contract TesterB {
error MyError(string msg);
function testError(bool path) public view {
 if(path) revert MyError("test error");
}
}

Gas saving executing: 9 per entry

TesterA.testRevert: 21611
TesterB.testError:  21602     

Affected source code:

• ERC20PermitPermissionedMint.sol:41
• ERC20PermitPermissionedMint.sol:46
• ERC20PermitPermissionedMint.sol:66
• ERC20PermitPermissionedMint.sol:68
• ERC20PermitPermissionedMint.sol:77
• ERC20PermitPermissionedMint.sol:78
• ERC20PermitPermissionedMint.sol:95
• OperatorRegistry.sol:46
• OperatorRegistry.sol:137
• OperatorRegistry.sol:182
• OperatorRegistry.sol:203
• frxETHMinter.sol:79
• frxETHMinter.sol:87
• frxETHMinter.sol:88
• frxETHMinter.sol:122
• frxETHMinter.sol:126
• frxETHMinter.sol:140
• frxETHMinter.sol:160
• frxETHMinter.sol:167
• frxETHMinter.sol:171
• frxETHMinter.sol:193
• frxETHMinter.sol:200

Total gas saved: 9 * 22 = 198

9. Optimize xERC4626.totalAssets

It is possible to reduce the definition of the variables in this way:

    function totalAssets() public view override returns (uint256) {
-       // cache global vars
-       uint256 storedTotalAssets_ = storedTotalAssets;
-       uint192 lastRewardAmount_ = lastRewardAmount;
        uint32 rewardsCycleEnd_ = rewardsCycleEnd;
-       uint32 lastSync_ = lastSync;

        if (block.timestamp >= rewardsCycleEnd_) {
            // no rewards or rewards fully unlocked
            // entire reward amount is available
-           return storedTotalAssets_ + lastRewardAmount_;
+           return storedTotalAssets + lastRewardAmount;
        }

+       uint32 lastSync_ = lastSync;

        // rewards not fully unlocked
        // add unlocked rewards to stored total
-       uint256 unlockedRewards = (lastRewardAmount_ * (block.timestamp - lastSync_)) / (rewardsCycleEnd_ - lastSync_);
-       return storedTotalAssets_ + unlockedRewards;
+       uint256 unlockedRewards = (lastRewardAmount * (block.timestamp - lastSync_)) / (rewardsCycleEnd_ - lastSync_);
+       return storedTotalAssets + unlockedRewards;
    }

• xERC4626.sol:45-61

10. Change bool to uint256 can save gas

Because each write operation requires an additional SLOAD to read the slot's contents, replace the bits occupied by the boolean, and then write back, booleans are more expensive than uint256 or any other type that uses a complete word. This cannot be turned off because it is the compiler's defense against pointer aliasing and contract upgrades.

Reference:

• https://github.com/OpenZeppelin/openzeppelin-contracts/blob/master/contracts/security/ReentrancyGuard.sol#L23-L27

Also, this is applicable to integer types different from uint256 or int256.

Affected source code for booleans:

• ERC20PermitPermissionedMint.sol:20
• frxETHMinter.sol:43
• frxETHMinter.sol:49
• frxETHMinter.sol:50

11. Argument not needed

The approveMax argument is not necessary since the maximum amount can already be set in the assets or shares argument by the caller.

Affected source code:

Remove bool approveMax:

• sfrxETH.sol:63
• sfrxETH.sol:79