Frax Ether Liquid Staking contest - csanuragjain's results

Lines of code

https://github.com/code-423n4/2022-09-frax/blob/main/src/frxETHMinter.sol#L191

Vulnerability details

Impact

Normally Admin should only be allowed to transfer upto currentWithheldETH to himself. But using recoverEther function, Admin can transfer all Ether (which were meant for validators) to himself

Proof of Concept

1. Observe the recoverEther function

function recoverEther(uint256 amount) external onlyByOwnGov {
        (bool success,) = address(owner).call{ value: amount }("");
        require(success, "Invalid transfer");

        emit EmergencyEtherRecovered(amount);
    }

2. Observe there is no restriction placed on this function and governance can use this to extract all ether which were meant to be used for validators

Recommended Mitigation Steps

Remove this function if not required

Lines of code

https://github.com/corddry/ERC4626/blob/643cd044fac34bcbf64e1c3790a5126fec0dbec1/src/xERC4626.sol#L37

Vulnerability details

Impact

In constructor of xERC4626.sol, there is no check to validate if rewardsCycleLength<= block.timestamp.safeCastTo32(). This can cause the full reward distribution to fail since rewardsCycleEnd will become 0 in this case

Proof of Concept

1. xERC4626.sol is deployed with _rewardsCycleLength as 2000000000
2. rewardsCycleEnd gets calculated like below

rewardsCycleEnd = (block.timestamp.safeCastTo32() / rewardsCycleLength) * rewardsCycleLength;

3. Since division is happening first so this becomes :

rewardsCycleEnd= (1663949858/2000000000)*2000000000 = 0*2000000000=0

4. Since rewardsCycleEnd is 0, syncRewards which is responsible for addition of reward will fail due to division by zero

uint32 end = ((timestamp + rewardsCycleLength) / rewardsCycleLength) * rewardsCycleLength;
// Since rewardsCycleLength is 0 so this fails as division by zero

Recommended Mitigation Steps

In constructor add a new check like below

require(rewardsCycleLength<= block.timestamp.safeCastTo32(), "Invalid reward cycle");