Asymmetry contest - ArbitraryExecution's results

Lines of code

https://github.com/code-423n4/2023-03-asymmetry/blob/main/contracts/SafEth/SafEth.sol#L63

Vulnerability details

Ether can get stuck in staking contract if there are no derivatives

Impact

If the SafEth contract is deployed and there are no derivatives added to the contract and a user tries to call the stake function, then this could result in a loss of funds for the user.

Proof of Concept

The following scenario could occur, which would result in the user having their Ether locked in the contract with no way of recovering their funds:

1. user calls the stake function and sends 1 ether to stake
2. there are no derivatives set in the contract so derivativeCount is 0
3. this means the for-loop will not run and totalStakeValueEth will equal 0
4. as a result, the user will be minted 0 safETH erc20 tokens
5. the stake function call will successfully go through, resulting in the user sending 1 ether to the contract, but receives 0 tokens

The following test can be written in the SafEth-Integration.test.ts file to confirm this:

  it("should result in user losing funds, while receiving 0 safEth tokens", async function () {
    // get SafEth contract
    const strategy = await getLatestContract(strategyContractAddress, "SafEth");

    // get Bob's ether balance before staking
    const [bob] = await ethers.getSigners();
    console.log("bob balance before: ", await ethers.provider.getBalance(bob.address));

    // get Bob's safEth token balance before staking
    let bobTokensBefore = await strategy.connect(bob).balanceOf(bob.address);
    console.log("bob tokens before", bobTokensBefore)

    // Bob tries to stake 1 Ether when there are no derivative contracts added
    // get Bob's safEth token balance before staking
    await strategy.connect(bob).stake({value: ethers.utils.parseEther("1")});

    // Bob's Ether balance will have decreased by at least 1 Ether
    console.log("bob balance after: ", await ethers.provider.getBalance(bob.address));

    // Bob's safEth token's balance will remain unchanged, meaning Bob was minted 0 tokens for staking 1 Ether
    let bobTokensAfter = await strategy.connect(bob).balanceOf(bob.address);
    console.log("bob tokens after", bobTokensAfter)

  });

Tools Used

VS Code, Hardhat

Recommended Mitigation Steps

Consider pausing the staking for the SafEth contract in the initializer function and only unpause the SafEth contract once derivatives have been added.

Floating pragma

Description

Contracts should be deployed with the same compiler version and flags that they have been tested with thoroughly. Locking the pragma helps to ensure that contracts do not accidentally get deployed using, for example, an outdated compiler version that might introduce bugs that affect the contract system negatively. https://swcregistry.io/docs/SWC-103

Floating Pragma List: SafeEth.sol - ^0.8.13 Reth.sol - ^0.8.13 SfrxEth.sol - ^0.8.13 WstEth.sol - ^0.8.13

Recommendation

Consider locking the compiler pragma to the specific version of the Solidity compiler used during testing and development.

Shadowed variables

References

• SafEth.sol#77

Description

In the stake function in SafEth.sol, a uint256 totalSupply variable is defined and shadows the existing totalSupply() function. This can be dangerous as any following code within that context will lose the ability to call the totalSupply() function. It is best practice to avoid shadowing variables and functions.

Recommendation

Consider instead prefixing the local totalSupply variable with an underscore to avoid shadowing the totalSupply() function

Users can lock funds after contract initialization

References

• SafEth.sol#48-56
• OwnableUpgradeable#29

Description

In the initialize function for SafEth.sol, the _transferOwnership() function is invoked to transfer the ownership to the deployer. The OpenZeppelin OwnableUpgradeable library includes a __Ownable_init function which has the same functionality but is intended to be used during initialization.

Recommendation

Consider replacing the _transferOwnership() call with __Ownable_init() in the initialize function to adhere to best practices set by the owners of the OwnableUpgradeable library.

Unused Import

References

• Reth.sol#5

Description

The IsFrxEth.sol contract is imported in Reth.sol but remains unused.

Recommendation

Consider removing all unused imports in order to keep code clean and readable.

Unnecessary Casts

References

• SfrxEth.sol#61
• SfrxEth.sol#98
• SfrxEth.sol#102
• SfrxEth.sol#112
• SfrxEth.sol#123
• WstEth.sol#57
• WstEth.sol#74
• WstEth.sol#78
• WstEth.sol#87
• WstEth.sol#94
• WstEth.sol#58
• WstEth.sol#59

Description

• To improve code clarity, the SFRX_ETH_ADDRESS and FRX_ETH_ADDRESS in SfrxEth.sol could be declared as a IsFrxEth type. Additionally, a balanceOf(address) function could be added to IsFrxEth.sol. This would remove the need for IsFrxEth and IERC20 casts.

• To improve code clarity, the WST_ETH in WstEth.sol could be declared as a IWStETH type. This would remove the need for IWStETH and IERC20 casts.

• To improve code clarity the STETH_TOKEN in WstEth.sol could be declared as a IERC20 type. This would remove the need for IERC20 casts.

Recommendation

Consider saving the aforementioned addresses as their interface counterparts, as well as including any required functions or interfaces to the existing interfaces.

Lacking checks around derivative contracts

References

• SafEth.sol#182

Description

In the addDerivative function of the SafeEth contract, there is not a check to see if the _contractAddress parameter is actually a contract addresss.

Adding any deployed contract address that do not conform to the IDerivative interface can prevent the whole protocol from working.

Recommendation

Consider checking for codeAt any derivative contract added with the addDerivative function, or implementing an ERC165 supports interface to ensure only valid derivative contracts are added.

Updates can be called on non-existent derivatives

References

• `SafEth#165
• SafEth.sol#202

Description

Both the adjustWeight and setMaxSlippage functions can be called on non-existent derivatives.

Recommendation

In order to restrict inputs into the contract, consider requiring the index set to be a valid derivative for both the adjustWeight and setMaxSlippage functions.

approve external call result not checked

References

[SfrxEth.sol#69-72](https://github.com/code-423n4/2023-03-asymmetry/blob/44b5cd94ebedc187a08884a7f685e950e987261c/contracts/SafEth/derivatives/SfrxEth.sol#L69-L72 [WstEth.sol#59](https://github.com/code-423n4/2023-03-asymmetry/blob/44b5cd94ebedc187a08884a7f685e950e987261c/contracts/SafEth/derivatives/WstEth.sol#L59 Reth.sol#90

Description

The withdraw function in SfrxEth.sol calls the approve function on FRX_ETH_ADDRESS but the return value is not checked to ensure success The withdraw function in WstEth.sol calls the approve function on STETH_TOKEN but the return value is not checked to ensure success The swapExactInputSingleHop function in Reth.sol calls the approve function on UNISWAP_ROUTER but the return value is not checked to ensure success

Recommendation

Consider instead prefixing the local totalSupply variable with an underscore to avoid shadowing the totalSupply() function

Usage of uint

Description

Throughout the SafEth.sol contract and in one place in the Reth.sol contract, the uint term is used instead of uint256.

Recommendation

It is recommended to use the uint256 keyword over the uint keyword for explicitness. Consider changing all instances of uint to uint256 to keep the code consistent.

Token decimals may change and break withdraw logic

Description

In the withdraw function in WstEth.sol and SfrxEth.sol the value 10 ** 18 is hardcoded and used to calculate the correct number of minimum tokens. This currently works as expected but the STETH_TOKEN address is a proxy and as such it is possible the decimals field may change in the future which would break the calculation and could set minOut to a value that is too high or too low.

Redundant external balanceOf call

Gas savings in the deposit function in SfrxEth.sol can be had by returning the return value from the submitAndDeposit external call. This would eliminate two external balanceOf external calls, as well as the subtraction operation.

Redundant external balanceOf call

Gas savings in the withdraw function in WstEth.sol can be had by saving off the return value of the unwrap call and using that return value instead of making an additional balanceOf external call to the STETH_TOKEN address.

Use an Array instead of a mapping

The following loop could be made more gas efficient by changing the derivatives variable type:

    for (uint i = 0; i < derivativeCount; i++)
        underlyingValue +=
            (derivatives[i].ethPerDerivative(derivatives[i].balance()) *
                derivatives[i].balance()) /
            10 ** 18;

A storage pointer can't be used here because its a mapping so the elements won't be contiguous. If the derivatives variable is changed from a mapping to an array a storage pointer could be used and would result in significant gas savings.