Frax Ether Liquid Staking contest - Soosh's results

Lines of code

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

Vulnerability details

• The depositEther() function should deposit into the depositContract if numDeposits > 0 and numValidators() > 0.
• It is expected that if there are enough validators N and enough Ether, then the deposit should go through for those N validators.
• However, the current implementation makes it so that if there is N validators and M numDeposits with N < M, then transaction will revert in getNextValidator() and no deposits will be made to the depositContract.

for (uint256 i = 0; i < numDeposits; ++i) {
            // Get validator information
            (
                bytes memory pubKey,
                bytes memory withdrawalCredential,
                bytes memory signature,
                bytes32 depositDataRoot
            ) = getNextValidator(); // Will revert if there are not enough free validators

• Example:
  • Validators N = 3, numDeposits M = 4
  • First 3 loops, getNextValidator() and full loop runs smoothly
  • After 3 loops, M=1, N=0, there are no validators left but still a deposit left so fourth loop is ran.
  • This will cause getNextValidator() to revert since the validator stack will be empty.

  require(numVals != 0, "Validator stack is empty");

  • Because the transaction is reverted, none of the deposits will go through even though there were enough validators in the stack for 3 deposits.

POC

 function testDepositEtherDOS() public {
        vm.startPrank(FRAX_COMPTROLLER);
        // Add a validator
        minter.addValidator(OperatorRegistry.Validator(pubKeys[0], sigs[0], ddRoots[0]));
        // Give the comptroller 320 ETH
        vm.deal(FRAX_COMPTROLLER, 320 ether);
        // Deposit 64 ETH for frxETH
        minter.submit{ value: 64 ether }();
        vm.expectRevert("Validator stack is empty");
        minter.depositEther();

        vm.stopPrank();
    }

• Add this test case to frxETHMinterTest contract to test for the vulnerability.

forge test -vv --match-contract frxETHMinter --fork-url "https://rpc.ankr.com/eth"

Impact

• Denial of service: being unable to activate validators when there are enough validators on the stack and enough deposits.

Recommendations

• A possible solution is to use the minumum of the numDeposits and validator lengths for the loop condtion

require(numDeposits > 0, "Not enough ETH in contract");
...
        uint _numValidators = numValidators();
        uint counter = numDeposits < _numValidators ? numDeposits : _numValidators;
        // Give each deposit chunk to an empty validator
        for (uint256 i = 0; i < counter; ++i) {
...

• Note that this solution will NOT revert on getNextValidator() as the loop will not be entered in the first place if either numValidators() or numDeposits is zero. This will break the existing testSubmitAndDepositEther() test case (expects to revert).

Lines of code

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

Vulnerability details

• The sfrxETH ERC-20 contract does not have protections against frontrunning approve/transferFrom methods.
• Possible attack scenario:
  • Alice approves() Bob to spend 5 sfrxETH
  • Alices decides to approve() Bob to spend 10 sfrxETH instead
  • Bob frontruns Alice's approve() transaction by spending the first 5 sfrxETH approval before the new approve() for 10 sfrxETH is called
  • Now, Bob will still have 10 sfrxETH approval from Alice and can spend it
  • Outcome: Bob can spend 15 sfrxETH when Alice expected for Bob to only spend 10 sfrxETH
• Reference: https://docs.google.com/document/d/1YLPtQxZu1UAvO9cZ1O2RPXBbT0mooh4DYKjA_jp-RLM/edit

Impact

• Loss of funds for a user due to subverted expectations

Recommendations

• Implement the increaseAllowance() and decreaseAllowance() functions for users to safely adjust the allowance for another user. This is standard if using OZ's ERC20.sol contract

PS

• ERC20 is from solmate contracts. So maybe OOS. Reporting because it affects the in-scope item sfrxETH.