Frax Ether Liquid Staking contest - PaludoX0's results

Lines of code

https://github.com/code-423n4/2022-09-frax/blob/55ea6b1ef3857a277e2f47d42029bc0f3d6f9173/src/ERC20/ERC20PermitPermissionedMint.sol#L59

Vulnerability details

Impact

Function minter_mint can be called by allowed minters, but only frxETHMinter contract should be allowed to mint token otherwise peg would lost if frxETH are minted without transferring equivalent ETH. If an attacker gains ownership can mint frxETH without depositing ETH.

Tools Used

Visual Studio review

Recommended Mitigation Steps

The ERC20PermitPermissionedMint shall be changed by setting minters to a single address that could be changed by a Timelock contract or a multisig wallet only

https://github.com/code-423n4/2022-09-frax/blob/55ea6b1ef3857a277e2f47d42029bc0f3d6f9173/src/frxETHMinter.sol#L166 It has to be checked by means of "require" that "address payable to" != 0x000 address otherwise funds could be lost forever

https://github.com/corddry/ERC4626/blob/643cd044fac34bcbf64e1c3790a5126fec0dbec1/src/xERC4626.sol#L60 There'a a very unlikely case that rewardsCycleEnd_ = lastSync_ and function will revert. This makes call revert without giving a clear error.

https://github.com/code-423n4/2022-09-frax/blob/55ea6b1ef3857a277e2f47d42029bc0f3d6f9173/src/frxETHMinter.sol#L129 https://github.com/code-423n4/2022-09-frax/blob/55ea6b1ef3857a277e2f47d42029bc0f3d6f9173/src/OperatorRegistry.sol#L62 https://github.com/code-423n4/2022-09-frax/blob/55ea6b1ef3857a277e2f47d42029bc0f3d6f9173/src/OperatorRegistry.sol#L84 https://github.com/code-423n4/2022-09-frax/blob/55ea6b1ef3857a277e2f47d42029bc0f3d6f9173/src/OperatorRegistry.sol#L114 These increment operations never need to be checked for over/underflow because the variable will never reach the max number of uint256 (will run out of gas long before that happens). Every for loop can be rewritten as follows:

for(uint256 i ; i < x){
      //loop logic
      unchecked {++i;} }

https://github.com/code-423n4/2022-09-frax/blob/55ea6b1ef3857a277e2f47d42029bc0f3d6f9173/src/OperatorRegistry.sol#L114 original_validators.length shall be cached before for cycle

https://github.com/corddry/ERC4626/blob/643cd044fac34bcbf64e1c3790a5126fec0dbec1/src/xERC4626.sol#L78 function syncRewards can be rewritten as follows, which will save around 150 gas at each call

function syncRewards() public virtual {
    uint192 lastRewardAmount_ = lastRewardAmount;
    uint32 timestamp = block.timestamp.safeCastTo32();

    if (timestamp < rewardsCycleEnd) revert SyncError();

    uint256 nextRewards = asset.balanceOf(address(this)) - storedTotalAssets;

    uint32 end = ((timestamp + rewardsCycleLength) / rewardsCycleLength) * rewardsCycleLength;

    // Combined single SSTORE
    lastRewardAmount = nextRewards.safeCastTo192();
    lastSync = timestamp;
    rewardsCycleEnd = end;

    emit NewRewardsCycle(end, nextRewards);
}

https://github.com/code-423n4/2022-09-frax/blob/55ea6b1ef3857a277e2f47d42029bc0f3d6f9173/src/frxETHMinter.sol#L38 and 39 Use private instead of public for constants, it saves gas

https://github.com/code-423n4/2022-09-frax/blob/55ea6b1ef3857a277e2f47d42029bc0f3d6f9173/src/frxETHMinter.sol#L97 X += Y costs more gas than X = X + Y for state variables currentWithheldETH = currentWithheldETH + withheld_amt is more efficient then currentWithheldETH += withheld_amt

https://github.com/code-423n4/2022-09-frax/blob/55ea6b1ef3857a277e2f47d42029bc0f3d6f9173/src/frxETHMinter.sol#L79 and 126 != 0 costs less gas compared to > 0 for unsigned integers in require statements with the optimizer enabled

https://github.com/code-423n4/2022-09-frax/blob/55ea6b1ef3857a277e2f47d42029bc0f3d6f9173/src/OperatorRegistry.sol#L93 it's better to verify at function begininng if remove_idx exists in order to save gas

https://github.com/code-423n4/2022-09-frax/blob/55ea6b1ef3857a277e2f47d42029bc0f3d6f9173/src/OperatorRegistry.sol#L82 it's better to verify at function begininng if times < validators.length