Frax Ether Liquid Staking contest - Trust's results

Lines of code

LOC: https://github.com/code-423n4/2022-09-frax/blob/55ea6b1ef3857a277e2f47d42029bc0f3d6f9173/src/sfrxETH.sol#L26

Vulnerability details

Description

When protocols hand out rewards to staked tokens, they must be careful to do so without leaving a large MEV opportunity, otherwise a bot could sandwich the increase of token value by minting shares and immediately redeeming them for a larger underlying amount. By doing this with a large sum of funds, attackers can enjoy a large % of the rewards while minimizing the profits of stakers considerably. xERC4626 addresses this issue and is designed to release staking rewards linearly during some defined cycle length. totalAssets() returns the previous matured assets added to the unlockedRewards from the current cycle:

uint256 unlockedRewards = (lastRewardAmount_ * (block.timestamp - lastSync_)) / (rewardsCycleEnd_ - lastSync_); // reward * cycle elapsed time / cycle total time
return storedTotalAssets_ + unlockedRewards;

syncRewards() should be triggered externally and updates the cycle parameters:

uint32 end = ((timestamp + rewardsCycleLength) / rewardsCycleLength) * rewardsCycleLength;
…
lastSync = timestamp;
rewardsCycleEnd = end;

syncRewards() rounds timestamp to the next multiple of rewardsCycleLength and marks that timestamp as end of unlocked rewards. The problem is that timestamp could already be very close to the next rewardsCycleLength multiple, and therefore the release could be extremely sharp (Up to the entire reward amount released in a single block). syncRewards() is permissionless by design, so attacker can wait for timestamp to be close to the target and then call syncRewards(). This will only work if no one else called syncRewards() to update the current cycle.

Impact

Attacker can theoretically consume up to the whole reward amount in a single block.

Proof of Concept

Assume rewardCycleLength = 1000 seconds, current assets = 200 frxETH, current shares = 150 sfrxETH, reward = 20 frxETH, next block timestamp = 19999 1. Attacker calls syncRewards() -> end = ((19999 + 1000) / 1000 ) * 1000 = 20000 2. Attacker mints 150 sfrxETH shares , which costs 200 frxETH 3. Attacker waits 1 block 4. Attacker redeems 150 sfrxETH shares, for half of the total assets = (200 + 200 + 20) / 2 = 210 5. Attacker profits 10 ETH (50% of the reward) from a single block. Staking APY is cut in half. The more attacker deposits, the larger the % of reward consumed.

Tools Used

Manual audit

Recommended Mitigation Steps

syncRewards() should add a check: timestamp % rewardsCycleLength <= rewardsCycleLength * SYNC_THRESHOLD / SYNC_THRESHOLD_FACTOR, with a reasonable threshold. Since syncRewards() is used in beforeWithdraw, it's best to not revert but instead return a false value if we have missed the current cycle.

Lines of code

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

Vulnerability details

Description

depositEther() is used to distribute ETH funds to available validators. If there are more deposits to hand out than there are available validators, the function will revert() and no deposit will be issued. This has unintended consequences. Attacker can launch a grief attack, continually depositing more 32 ETH batches than the number of available validators and selling off the frxETH that was minted. It will freeze the validator matching system. Even if new validators are spun up, attacker can frontrun the depositEther() function and always make sure there is more ETH to distribute than validators. A side effect is that sfrxETH APY will decrease, because frxETH supply increased, but the supply mismatches the actual ETH used to farm validation rewards.

Impact

Attacker can disrupt matching of validators and harm the APY of sfrxETH.

Tools Used

Manual audit

Recommended Mitigation Steps

Wrap the getNextValidator() call with a try/catch block, and if there are no more available validators, return from the call successfully. This will improve the APY of the system regardless of security considerations.

Lines of code

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

Vulnerability details

Description

sfrxETH supports minting and depositing with a signature, which spares the user from approving the sfrxETH vault contract to use their frxETH. The permit function used in mintWithSignature(), verifies the supplied signature matches the fields: 1. Owner - msg.sender 2. Spender - this 3. Amount - either MAX_UINT or previewMint(shares) 4. Deadline - deadline 5. Owner nonce If user does not pass approveMax, which is the security-conscious option, the amount passed to permit is previewMint(shares). However, previewMint(shares) may well return a different amount from the amount used by the user to calculated the signature. previewMint() is supposed to calculate the amount of assets required to be passed to mint shares of the vault, rounded up. If, from the time user created the signature, to the time the signature is verified in mintWithSignature(), the amount changes, the signature check will fail and the TX would revert. The two amounts will likely be different if the protocol is in a unlocking rewards cycle. They could also different if a mint or redeem occurred and due to rounding, the result has changed.

Impact

User's call to mintWithSignature could fail through no fault of their own.

Proof of Concept

1. User would like to mint 100 shares, which are currently worth 110 assets. They sign a permit for 110 assets.
2. Permit signature is validated on the next block. Additional rewards have accrued, and now 100 shares are worth 110.1 assets. The signature fails (previewMint rounded up).

Tools Used

Manual audit

Recommended Mitigation Steps

approveMax variable should be a uint256 approveAmount, which is determined only during signature creation on the user's side. It should take into account the current reward cycle and future-proof the transaction with a higher than currently required amount.

Lines of code

https://github.com/code-423n4/2022-09-frax/blob/55ea6b1ef3857a277e2f47d42029bc0f3d6f9173/src/OperatorRegistry.sol#L140-L141

Vulnerability details

Description

getNextValidator() is used to get the next available validator to be assigned in depositEther(). The validators array is used as a stack, where addValidator adds a validator to the end of the array and getNextValidator() pops it from there. The consequence is that validators are matched out of order, which makes the system unfair in terms of the time validator has to wait before being assigned. In a possible scenario, the first validators will never be matched because additional validators will cut it in line. Also, because of another bug where if number of deposits available is greater than available validators, no validator is matched, there is less of a chance the early validators would be matched. This is because for them to be matched, there needs to be exactly enough 32 ETH multiples to pop all validators, without overrunning the array. Note that in a decentralized system, users must have the ability to join as legitimate validators, so the matching order must accommodate for both user supplied validators and in-house validators.

Impact

Validators may spend an arbitrary amount of time waiting to be matched.

Tools Used

Manual audit.

Recommended Mitigation Steps

getNextValidator should pop a validator from the beginning of the array. This could be done efficiently using a Yul snippet, which decrements the array length and stores it in the slot above.