Asymmetry contest - 0xAgro's results

Lines of code

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

Vulnerability details

Impact

If a bugged or malicious derivative contract were added, funds can be locked.

Proof of Concept

The unstake function seen below loops over all derivatives, if one derivative function reverts the function will always revert - hence pausing unstaking.

108:    function unstake(uint256 _safEthAmount) external {
                                      ...
113:        for (uint256 i = 0; i < derivativeCount; i++) {
114:            // withdraw a percentage of each asset based on the amount of safETH
115:            uint256 derivativeAmount = (derivatives[i].balance() *
116:                _safEthAmount) / safEthTotalSupply;
117:            if (derivativeAmount == 0) continue; // if derivative empty ignore
118:            derivatives[i].withdraw(derivativeAmount);
119:        }
                                      ...
129:    }

Tools Used

Manual Review

Recommended Mitigation Steps

Consider adding a removeDerivative function.

QA Report

Finding Summary

Low Severity

1. Compiler With Known Bug May Be Used
2. minAmount maxAmount Input Sanitization
3. Single Step Owner Transfer
4. Gas Grief If Relayed
5. Owner Can Bypass setPauseStaking

Non-Critical

1. Explicit Variable Not Used
2. bytes.concat() Can Be Used Over abi.encodePacked()
3. Inconsistent Named Returns
4. Spelling Mistake
5. Unnecessary Collapsed Code
6. Inconsistent Single Line If Style
7. Inconsistent Trailing Period
8. Inconsistent Comment Capitalization
9. Constant Not Used
10. Inconsistent Exponentiation Style
11. Mimicked ERC20 Names
12. ERC20 Token Recovery
13. Unbounded Compiler Version

Style Guide Violations

1. Maximum Line Length
2. Order of Functions
3. Whitespace in Expressions
4. Function Declaration Style

Low Severity

1. Compiler With Known Bug May Be Used

All contracts in scope (ex) use a Solidity version of ^0.8.13. There is a known bug that is present in Solidity 0.8.13 as well as 0.8.14. Consider using a Solidity version past 0.8.14.

2. minAmount maxAmount Input Sanitization

When setting the state variables minAmount and maxAmount in SafEth.sol, there is no input sanitization to ensure minAmount <= maxAmount. See setMaxAmount and setMinAmount.

214:    function setMinAmount(uint256 _minAmount) external onlyOwner {
215:        minAmount = _minAmount;
216:        emit ChangeMinAmount(minAmount);
217:    }

223:    function setMaxAmount(uint256 _minAmount) external onlyOwner {
224:        maxAmount = _maxAmount;
225:        emit ChangeMaxAmount(maxAmount);
226:    }

There is no logical reason minAmount > maxAmount should be true, and admin error may make it so.

Consider adding a require check to make sure minAmount <= maxAmount when changing either variable.

3. Single Step Owner Transfer

All contracts in scope use openzeppelin's OwnableUpgradeable contract (ex) which uses a single step owner transfer seen here:

83:    function _transferOwnership(address newOwner) internal virtual {
84:        address oldOwner = _owner;
85:        _owner = newOwner;
86:        emit OwnershipTransferred(oldOwner, newOwner);
87:    }

Single step owner transfers are prone to admin error.

4. Gas Grief If Relayed

When using (bool sent, ) = address(msg.sender).call, leaving out the returnData does not prevent it from being copied to memory. If transactions were relayed, gas griefing is possible. Consider using assembly to prevent such a grief.

/contracts/SafEth/SafEth.sol

124:    (bool sent, ) = address(msg.sender).call{value: ethAmountToWithdraw}(

/contracts/SafEth/derivatives/WstEth.sol

63:    (bool sent, ) = address(msg.sender).call{value: address(this).balance}(
76:    (bool sent, ) = WST_ETH.call{value: msg.value}("");

/contracts/SafEth/derivatives/SfrxEth.sol

84:    (bool sent, ) = address(msg.sender).call{value: address(this).balance}(

/contracts/SafEth/derivatives/Reth.sol

110:    (bool sent, ) = address(msg.sender).call{value: address(this).balance}(

For more information please see this post.

5. Owner Can Bypass setPauseStaking

The setPauseStaking function is used to pause staking function which emits an event:

232:    function setPauseStaking(bool _pause) external onlyOwner {
233:        pauseStaking = _pause;
234:        emit StakingPaused(pauseStaking);
235:    }

The StakingPaused event can help users listen for if the protocol has paused staking; however, the owner can bypass this. The owner can add a derivative that forces a reversion in it's ethPerDerivative function. The stake function seen below loops over all derivatives, if one reverts the function will always revert - hence pausing staking.

63:    function stake() external payable {
                                      ...
70:        // Getting underlying value in terms of ETH for each derivative
71:        for (uint i = 0; i < derivativeCount; i++)
72:            underlyingValue +=
73:                (derivatives[i].ethPerDerivative(derivatives[i].balance()) *
74:                    derivatives[i].balance()) /
75:                10 ** 18;
                                      ...
101:    }

If the protocol is paused in the manner descibed above the honesty of the protocol is impacted.

Non-Critical

1. Explicit Variable Not Used

As described in the Solidity documentation:

"uint and int are aliases for uint256 and int256, respectively".

There are moments in the codebase where uint is used instead of the explicit uint256. It is best to be explicit with variable names to lessen confusion. Consider replacing instances of uint with uint256.

/contracts/SafEth/SafEth.sol

26:	event Staked(address indexed recipient, uint ethIn, uint safEthOut);
27:	event Unstaked(address indexed recipient, uint ethOut, uint safEthIn);
28:	event WeightChange(uint indexed index, uint weight);
31:	uint weight,
32:	uint index
92:	uint derivativeReceivedEthValue = (derivative.ethPerDerivative(
203:	uint _derivativeIndex,
204:	uint _slippage

2. bytes.concat() Can Be Used Over abi.encodePacked()

Consider using bytes.concat() instead of abi.encodePacked() in contracts with Solidity version >= 0.8.4.

/contracts/SafEth/derivatives/Reth.sol

70:	abi.encodePacked("contract.address", "rocketTokenRETH")
125:	abi.encodePacked("contract.address", "rocketDepositPool")
136:	abi.encodePacked(
162:	abi.encodePacked("contract.address", "rocketDepositPool")
191:	abi.encodePacked("contract.address", "rocketTokenRETH")
233:	abi.encodePacked("contract.address", "rocketTokenRETH")

3. Inconsistent Named Returns

Some functions use named returns and others do not. It is best for code clearity to keep a consistent style.

1. No contracts only have named returns (EX. returns(uint256 foo)).
2. The following contracts only have non-named returns (EX. returns(uint256)): WstEth.sol, and SfrxEth.sol.
3. The following contracts have both: Reth.sol.

4. Spelling Mistake

There is a spelling mistake in the codebase. Consider fixing all spelling mistakes.

contracts/SafEth/SafEth.sol

• The word derivatives is misspelled as derivates.

5. Unnecessary Collapsed Code

There are moments in the codebase where lines (if expanded) less than 120 characters are collapsed - thus not voiding the Solidity Style Guide. The code here and here are examples. Some instances of this may void the Solidity Style Guide (see later in the report). Consider expanding code that is less than 120 characters.

6. Inconsistent Single Line If Style

This block and this block use single line conditional statements; however, in both cases the if style differs from the else style. The if statements use a newline where the else statement does not. Consider using a single consistant style.

7. Inconsistent Trailing Period

Almost all comments do not have a trailing period, with the exception of this line and this line. Consider removing all trailing periods to maintain style.

8. Inconsistent Comment Capitalization

There is an inconsistency in the capitalization of comments. For example, this line is not capitilized, while this line is. Consider capitilizing all comments.

9. Constant Not Used

The value 10 ** 18 is used consistantly throughout the codebase. Consider using a constant DECIMALS in place of 10 ** 18.

10. Inconsistent Exponentiation Style

The value 10 ** 18 is used in the style seen for almost all instances in the codebase (ex). There is one instance where 10 ** 18 is in the style 1e18. Consider changing this style to 10 ** 18 or all instances to 1e18.

11. Mimicked ERC20 Names

The ERC20 Token names do not indicate they are a derivative contract and use the native token names. They should be more descriptive. Consider changing the token names to better reflect the use and not confuse users.

/contracts/SafEth/derivatives/WstEth.sol

41:    function name() public pure returns (string memory) {
42:        return "Lido";
43:    }

/contracts/SafEth/derivatives/SfrxEth.sol

44:    function name() public pure returns (string memory) {
45:        return "Frax";
46:    }

/contracts/SafEth/derivatives/Reth.sol

50:    function name() public pure returns (string memory) {
51:        return "RocketPool";
52:    }

12. ERC20 Token Recovery

Consider adding a recovery function for Tokens that are accidently sent to the core contracts of the protocol.

13. Unbounded Compiler Version

All contracts in scope use a compiler version of ^0.8.13. No one knows what will come in future compiler versions < 0.9.0, thus it is best to put an upper limit on the compiler version.

Style Guide Violations

1. Maximum Line Length

Lines with greater length than 120 characters are used. The Solidity Style Guide suggests that all lines should be 120 characters or less in width.

The following lines are longer than 120 characters, it is suggested to shorten these lines:

contracts/SafEth/SafEth.sol

• 160.

contracts/SafEth/derivatives/Reth.sol

• 142.

2. Order of Functions

The Solidity Style Guide suggests the following function order: constructor, receive function (if exists), fallback function (if exists), external, public, internal, private.

The following contracts are not compliant (examples are only to prove the functions are out of order NOT a full description):

• WstEth.sol: external functions are positioned after public functions.
• SfrxEth.sol: external functions are positioned after public functions.
• Reth.sol: external functions are positioned after public functions.

3. Whitespace in Expressions

The Solidity Style Guide recommends to

"[a]void extraneous whitespace [i]mmediately inside parenthesis, brackets or braces, with the exception of single line function declarations".

The Style Guide also recommends against spaces before semicolons. Consider removing spaces before semicolons.

The following do follow Solidity style conventions, but technically void the style guide.

/contracts/SafEth/derivatives/WstEth.sol

63:	(bool sent, ) = address(msg.sender).call{value: address(this).balance}(
76:	(bool sent, ) = WST_ETH.call{value: msg.value}("");

/contracts/SafEth/derivatives/SfrxEth.sol

84:	(bool sent, ) = address(msg.sender).call{value: address(this).balance}(

/contracts/SafEth/SafEth.sol

124:	(bool sent, ) = address(msg.sender).call{value: ethAmountToWithdraw}(

/contracts/SafEth/derivatives/Reth.sol

110:	(bool sent, ) = address(msg.sender).call{value: address(this).balance}(
240:	(uint160 sqrtPriceX96, , , , , , ) = pool.slot0();

4. Function Declaration Style

The Solidity Style Guide states that

"[f]or short function declarations, it is recommended for the opening brace of the function body to be kept on the same line as the function declaration".

It is not clear what length is exactly meant by "short" or "long". The maximum line length of 120 characters may be an indication, and in that case any expanded function declaration under 120 characters should be on a single line.

The following functions in their respective contracts are not compliant by this standard:

contracts/SafEth/SafEth.sol

• initialize, adjustWeight, addDerivative, setMaxSlippage.