QA Report - Githubissues

code423n4 commented 2 years ago

Summary

Low Risk Issues

	Issue	Instances
1	Slippage of 1% is too strict	1
2	Mistaken null values cause `distributeYield()` to revert	1
3	Can't remove old assets	1
4	Missing checks for `approve()`'s return status	1
5	Move `ETH` constant to child contract	1
6	Unsafe casts and usage of `IERC20Detailed`	1
7	Unused `receive()` function will lock Ether in contract	1
8	`safeApprove()` is deprecated	3
9	Missing checks for `address(0x0)` when assigning values to `address` state variables	1
10	Open TODOs	1

Total: 12 instances over 10 issues

Non-critical Issues

	Issue	Instances
1	`override` function arguments that are unused should have the variable name removed or commented out to avoid compiler warnings	2
2	`public` functions not called by the contract should be declared `external` instead	3
3	`constant`s should be defined rather than using magic numbers	6
4	Redundant cast	1
5	Missing event for critical parameter change	2
6	Use a more recent version of solidity	3
7	Use a more recent version of solidity	1
8	Variable names that consist of all capital letters should be reserved for `const`/`immutable` variables	1
9	NatSpec is incomplete	2
10	Event is missing `indexed` fields	4
11	Consider allowing the passing of a referral code	1
12	Remove commented out code	1
13	Consider two-phase ownership transfer	1

Total: 28 instances over 13 issues

Low Risk Issues

1. Slippage of 1% is too strict

The GeneralVault assumes that st tokens are always 1:1 redeemable and therefore the slippage should be negligable. This currently is not the case, specifically for LIDO where the depegging is >4%: https://twitter.com/LidoFinance/status/1525129266689622016 I've marked this as 'Low' rather than 'Medium' because funds can still be withdrawn as LIDO directly, and converted at a later point

There is 1 instance of this issue:

File: smart-contracts/GeneralVault.sol   #1

117      // Withdraw from vault, it will convert stAsset to asset and send to user
118      // Ex: In Lido vault, it will return ETH or stETH to user
119      uint256 withdrawAmount = _withdrawFromYieldPool(_asset, _amountToWithdraw, _to);
120  
121      if (_amount == type(uint256).max) {
122        uint256 decimal = IERC20Detailed(_asset).decimals();
123        _amount = _amountToWithdraw.mul(this.pricePerShare()).div(10**decimal);
124      }
125:     require(withdrawAmount >= _amount.percentMul(99_00), Errors.VT_WITHDRAW_AMOUNT_MISMATCH);

https://github.com/code-423n4/2022-05-sturdy/blob/78f51a7a74ebe8adfd055bdbaedfddc05632566f/smart-contracts/GeneralVault.sol#L117-L125

2. Mistaken null values cause `distributeYield()` to revert

There are no null checks in the registerAsset() function, so admins can mistakenly pass 0x0 to that function, which will cause the for-loop to revert when that asset is reached. I've marked this as 'Low' rather than 'Medium' since the admin can work around it by using the _offset and _count input arguments to the function.

There is 1 instance of this issue:

File: smart-contracts/YieldManager.sol   #1

118    function distributeYield(uint256 _offset, uint256 _count) external onlyAdmin {
119      // 1. convert from asset to exchange token via uniswap
120      for (uint256 i = 0; i < _count; i++) {
121        address asset = _assetsList[_offset + i];
122:       require(asset != address(0), Errors.UL_INVALID_INDEX);

https://github.com/code-423n4/2022-05-sturdy/blob/78f51a7a74ebe8adfd055bdbaedfddc05632566f/smart-contracts/YieldManager.sol#L118-L122

3. Can't remove old assets

There is no way to remove old assets added by calls to registerAsset(). A disgruntled admin, before their access is revoked, can add a lot of assets and regularly sprinkle them with dust, so the new admins have to submit multiple calls to distributeYield() with different offsets and counts, to avoid the dust and possible reversion due to running out of gas

There is 1 instance of this issue:

File: smart-contracts/YieldManager.sol   #1

118    function distributeYield(uint256 _offset, uint256 _count) external onlyAdmin {
119      // 1. convert from asset to exchange token via uniswap
120      for (uint256 i = 0; i < _count; i++) {
121        address asset = _assetsList[_offset + i];
122        require(asset != address(0), Errors.UL_INVALID_INDEX);
123        uint256 _amount = IERC20Detailed(asset).balanceOf(address(this));
124        _convertAssetToExchangeToken(asset, _amount);
125:     }

https://github.com/code-423n4/2022-05-sturdy/blob/78f51a7a74ebe8adfd055bdbaedfddc05632566f/smart-contracts/YieldManager.sol#L118-L125

4. Missing checks for `approve()`'s return status

Some tokens, such as Tether (USDT) return false rather than reverting if the approval fails. Use OpenZeppelin's safeApprove(), which reverts if there's a failure, instead

There is 1 instance of this issue:

File: smart-contracts/YieldManager.sol   #1

221:     IERC20(_asset).approve(_lendingPool, _amount);

https://github.com/code-423n4/2022-05-sturdy/blob/78f51a7a74ebe8adfd055bdbaedfddc05632566f/smart-contracts/YieldManager.sol#L221

5. Move `ETH` constant to child contract

All of the functions in the GeneralVault require 0x0 when referring to Ether, not the constant here. Having it here will lead to mistakes down the line. It's only used by CurveswapAdapter, so it only needs to be there (it currently is also defined there).

There is 1 instance of this issue:

File: smart-contracts/GeneralVault.sol   #1

47:    address constant ETH = 0xEeeeeEeeeEeEeeEeEeEeeEEEeeeeEeeeeeeeEEeE;

https://github.com/code-423n4/2022-05-sturdy/blob/78f51a7a74ebe8adfd055bdbaedfddc05632566f/smart-contracts/GeneralVault.sol#L47

6. Unsafe casts and usage of `IERC20Detailed`

The GeneralVault is meant to be general, i.e. not specific to Lido or Curve, and therefore should not assume that the asset will always be IERC20Detailed (not all ERC20 contracts define decimals() since it's optional in the spec). Use safeDecimals() instead

There is 1 instance of this issue:

File: smart-contracts/GeneralVault.sol   #1

122:       uint256 decimal = IERC20Detailed(_asset).decimals();

https://github.com/code-423n4/2022-05-sturdy/blob/78f51a7a74ebe8adfd055bdbaedfddc05632566f/smart-contracts/GeneralVault.sol#L122

7. Unused `receive()` function will lock Ether in contract

If the intention is for the Ether to be used, the function should call another function, otherwise it should revert

There is 1 instance of this issue:

File: smart-contracts/LidoVault.sol   #1

24:     receive() external payable {}

https://github.com/code-423n4/2022-05-sturdy/blob/78f51a7a74ebe8adfd055bdbaedfddc05632566f/smart-contracts/LidoVault.sol#L24

8. `safeApprove()` is deprecated

Deprecated in favor of safeIncreaseAllowance() and safeDecreaseAllowance(). If only setting the initial allowance to the value that means infinite, safeIncreaseAllowance() can be used instead

There are 3 instances of this issue:

File: smart-contracts/ConvexCurveLPVault.sol   #1

141:      IERC20(curveLPToken).safeApprove(convexBooster, _amount);

https://github.com/code-423n4/2022-05-sturdy/blob/78f51a7a74ebe8adfd055bdbaedfddc05632566f/smart-contracts/ConvexCurveLPVault.sol#L141

File: smart-contracts/ConvexCurveLPVault.sol   #2

146:      IERC20(internalAssetToken).safeApprove(address(_addressesProvider.getLendingPool()), _amount);

https://github.com/code-423n4/2022-05-sturdy/blob/78f51a7a74ebe8adfd055bdbaedfddc05632566f/smart-contracts/ConvexCurveLPVault.sol#L146

File: smart-contracts/LidoVault.sol   #3

102:      IERC20(LIDO).safeApprove(address(_addressesProvider.getLendingPool()), assetAmount);

https://github.com/code-423n4/2022-05-sturdy/blob/78f51a7a74ebe8adfd055bdbaedfddc05632566f/smart-contracts/LidoVault.sol#L102

9. Missing checks for `address(0x0)` when assigning values to `address` state variables

There is 1 instance of this issue:

File: smart-contracts/ConvexCurveLPVault.sol   #1

41:       curveLPToken = _lpToken;

https://github.com/code-423n4/2022-05-sturdy/blob/78f51a7a74ebe8adfd055bdbaedfddc05632566f/smart-contracts/ConvexCurveLPVault.sol#L41

10. Open TODOs

Code architecture, incentives, and error handling/reporting questions/issues should be resolved before deployment

There is 1 instance of this issue:

File: smart-contracts/GeneralVault.sol   #1

77:       // Ex: if user deposit 100ETH, this will deposit 100ETH to Lido and receive 100stETH TODO No Lido

https://github.com/code-423n4/2022-05-sturdy/blob/78f51a7a74ebe8adfd055bdbaedfddc05632566f/smart-contracts/GeneralVault.sol#L77

Non-critical Issues

1. `override` function arguments that are unused should have the variable name removed or commented out to avoid compiler warnings

There are 2 instances of this issue:

File: smart-contracts/ConvexCurveLPVault.sol   #1

154:    function _getWithdrawalAmount(address _asset, uint256 _amount)

https://github.com/code-423n4/2022-05-sturdy/blob/78f51a7a74ebe8adfd055bdbaedfddc05632566f/smart-contracts/ConvexCurveLPVault.sol#L154

File: smart-contracts/LidoVault.sol   #2

109:    function _getWithdrawalAmount(address _asset, uint256 _amount)

https://github.com/code-423n4/2022-05-sturdy/blob/78f51a7a74ebe8adfd055bdbaedfddc05632566f/smart-contracts/LidoVault.sol#L109

2. `public` functions not called by the contract should be declared `external` instead

Contracts are allowed to override their parents' functions and change the visibility from external to public.

There are 3 instances of this issue:

File: smart-contracts/CollateralAdapter.sol   #1

35:     function initialize(ILendingPoolAddressesProvider _provider) public initializer {

https://github.com/code-423n4/2022-05-sturdy/blob/78f51a7a74ebe8adfd055bdbaedfddc05632566f/smart-contracts/CollateralAdapter.sol#L35

File: smart-contracts/GeneralVault.sol   #2

61:     function initialize(ILendingPoolAddressesProvider _provider) public initializer {

https://github.com/code-423n4/2022-05-sturdy/blob/78f51a7a74ebe8adfd055bdbaedfddc05632566f/smart-contracts/GeneralVault.sol#L61

File: smart-contracts/YieldManager.sol   #3

60:     function initialize(ILendingPoolAddressesProvider _provider) public initializer {

https://github.com/code-423n4/2022-05-sturdy/blob/78f51a7a74ebe8adfd055bdbaedfddc05632566f/smart-contracts/YieldManager.sol#L60

3. `constant`s should be defined rather than using magic numbers

There are 6 instances of this issue:

File: smart-contracts/ConvexCurveLPVault.sol

/// @audit 0xF403C135812408BFbE8713b5A23a04b3D48AAE31
40:       convexBooster = 0xF403C135812408BFbE8713b5A23a04b3D48AAE31;

https://github.com/code-423n4/2022-05-sturdy/blob/78f51a7a74ebe8adfd055bdbaedfddc05632566f/smart-contracts/ConvexCurveLPVault.sol#L40

File: smart-contracts/GeneralVault.sol

/// @audit 99_00
125:      require(withdrawAmount >= _amount.percentMul(99_00), Errors.VT_WITHDRAW_AMOUNT_MISMATCH);

/// @audit 30_00
167:      require(_fee <= 30_00, Errors.VT_FEE_TOO_BIG);

https://github.com/code-423n4/2022-05-sturdy/blob/78f51a7a74ebe8adfd055bdbaedfddc05632566f/smart-contracts/GeneralVault.sol#L125

File: smart-contracts/LidoVault.sol

/// @audit 200
48:         200

/// @audit 1e18
73:       return 1e18;

/// @audit 200
136:          200

https://github.com/code-423n4/2022-05-sturdy/blob/78f51a7a74ebe8adfd055bdbaedfddc05632566f/smart-contracts/LidoVault.sol#L48

4. Redundant cast

The type of the variable is the same as the type to which the variable is being cast

There is 1 instance of this issue:

File: smart-contracts/LidoVault.sol   #1

140:        (bool sent, bytes memory data) = address(_to).call{value: receivedETHAmount}('');

https://github.com/code-423n4/2022-05-sturdy/blob/78f51a7a74ebe8adfd055bdbaedfddc05632566f/smart-contracts/LidoVault.sol#L140

5. Missing event for critical parameter change

There are 2 instances of this issue:

File: smart-contracts/ConvexCurveLPVault.sol   #1

37      function setConfiguration(address _lpToken, uint256 _poolId) external onlyAdmin {
38        require(internalAssetToken == address(0), Errors.VT_INVALID_CONFIGURATION);
39    
40        convexBooster = 0xF403C135812408BFbE8713b5A23a04b3D48AAE31;
41        curveLPToken = _lpToken;
42        convexPoolId = _poolId;
43        SturdyInternalAsset _interalToken = new SturdyInternalAsset(
44          string(abi.encodePacked('Sturdy ', IERC20Detailed(_lpToken).symbol())),
45          string(abi.encodePacked('c', IERC20Detailed(_lpToken).symbol())),
46          IERC20Detailed(_lpToken).decimals()
47        );
48        internalAssetToken = address(_interalToken);
49:     }

https://github.com/code-423n4/2022-05-sturdy/blob/78f51a7a74ebe8adfd055bdbaedfddc05632566f/smart-contracts/ConvexCurveLPVault.sol#L37-L49

File: smart-contracts/YieldManager.sol   #2

64      function setExchangeToken(address _token) external onlyAdmin {
65        require(_token != address(0), Errors.VT_INVALID_CONFIGURATION);
66        _exchangeToken = _token;
67:     }

https://github.com/code-423n4/2022-05-sturdy/blob/78f51a7a74ebe8adfd055bdbaedfddc05632566f/smart-contracts/YieldManager.sol#L64-L67

6. Use a more recent version of solidity

Use a solidity version of at least 0.8.13 to get the ability to use using for with a list of free functions

There are 3 instances of this issue:

File: smart-contracts/GeneralVault.sol   #1

2:    pragma solidity 0.6.12;

https://github.com/code-423n4/2022-05-sturdy/blob/78f51a7a74ebe8adfd055bdbaedfddc05632566f/smart-contracts/GeneralVault.sol#L2

File: smart-contracts/LidoVault.sol   #2

2:    pragma solidity 0.6.12;

https://github.com/code-423n4/2022-05-sturdy/blob/78f51a7a74ebe8adfd055bdbaedfddc05632566f/smart-contracts/LidoVault.sol#L2

File: smart-contracts/YieldManager.sol   #3

2:    pragma solidity 0.6.12;

https://github.com/code-423n4/2022-05-sturdy/blob/78f51a7a74ebe8adfd055bdbaedfddc05632566f/smart-contracts/YieldManager.sol#L2

7. Use a more recent version of solidity

Use a solidity version of at least 0.8.4 to get bytes.concat() instead of abi.encodePacked(<bytes>,<bytes>) Use a solidity version of at least 0.8.12 to get string.concat() instead of abi.encodePacked(<str>,<str>)

There is 1 instance of this issue:

File: smart-contracts/ConvexCurveLPVault.sol   #1

2:    pragma solidity 0.6.12;

https://github.com/code-423n4/2022-05-sturdy/blob/78f51a7a74ebe8adfd055bdbaedfddc05632566f/smart-contracts/ConvexCurveLPVault.sol#L2

8. Variable names that consist of all capital letters should be reserved for `const`/`immutable` variables

If the variable needs to be different based on which class it comes from, a view/pure function should be used instead (e.g. like this).

There is 1 instance of this issue:

File: smart-contracts/LidoVault.sol   #1

127:      address LIDO = _addressesProvider.getAddress('LIDO');

https://github.com/code-423n4/2022-05-sturdy/blob/78f51a7a74ebe8adfd055bdbaedfddc05632566f/smart-contracts/LidoVault.sol#L127

9. NatSpec is incomplete

There are 2 instances of this issue:

File: smart-contracts/GeneralVault.sol   #1

/// @audit Missing: '@return'
134      * @param _amount The amount to be withdrawn
135      */
136     function withdrawOnLiquidation(address _asset, uint256 _amount)
137       external
138       virtual
139:      returns (uint256)

https://github.com/code-423n4/2022-05-sturdy/blob/78f51a7a74ebe8adfd055bdbaedfddc05632566f/smart-contracts/GeneralVault.sol#L134-L139

File: smart-contracts/YieldManager.sol   #2

/// @audit Missing: '@return'
104      * @param _tokenOut The address of token being received
105      */
106:    function getCurvePool(address _tokenIn, address _tokenOut) external view returns (address) {

https://github.com/code-423n4/2022-05-sturdy/blob/78f51a7a74ebe8adfd055bdbaedfddc05632566f/smart-contracts/YieldManager.sol#L104-L106

10. Event is missing `indexed` fields

Each event should use three indexed fields if there are three or more fields

There are 4 instances of this issue:

File: smart-contracts/GeneralVault.sol   #1

24:     event ProcessYield(address indexed collateralAsset, uint256 yieldAmount);

https://github.com/code-423n4/2022-05-sturdy/blob/78f51a7a74ebe8adfd055bdbaedfddc05632566f/smart-contracts/GeneralVault.sol#L24

File: smart-contracts/GeneralVault.sol   #2

25:     event DepositCollateral(address indexed collateralAsset, address indexed from, uint256 amount);

https://github.com/code-423n4/2022-05-sturdy/blob/78f51a7a74ebe8adfd055bdbaedfddc05632566f/smart-contracts/GeneralVault.sol#L25

File: smart-contracts/GeneralVault.sol   #3

26:     event WithdrawCollateral(address indexed collateralAsset, address indexed to, uint256 amount);

https://github.com/code-423n4/2022-05-sturdy/blob/78f51a7a74ebe8adfd055bdbaedfddc05632566f/smart-contracts/GeneralVault.sol#L26

File: smart-contracts/GeneralVault.sol   #4

27:     event SetTreasuryInfo(address indexed treasuryAddress, uint256 fee);

https://github.com/code-423n4/2022-05-sturdy/blob/78f51a7a74ebe8adfd055bdbaedfddc05632566f/smart-contracts/GeneralVault.sol#L27

11. Consider allowing the passing of a referral code

There is 1 instance of this issue:

File: smart-contracts/GeneralVault.sol   #1

81       ILendingPool(_addressesProvider.getLendingPool()).deposit(
82         _stAsset,
83         _stAssetAmount,
84         msg.sender,
85         0
86:      );

https://github.com/code-423n4/2022-05-sturdy/blob/78f51a7a74ebe8adfd055bdbaedfddc05632566f/smart-contracts/GeneralVault.sol#L81-L86

12. Remove commented out code

There is 1 instance of this issue:

File: smart-contracts/GeneralVault.sol   #1

144    // /**
145    //  * @dev Convert an `amount` of asset used as collateral to swappable asset on liquidation.
146    //  * @param _amountIn The amount of collateral asset
147    //  */
148:   // function convertOnLiquidation(address _assetOut, uint256 _amountIn) external virtual {}

https://github.com/code-423n4/2022-05-sturdy/blob/78f51a7a74ebe8adfd055bdbaedfddc05632566f/smart-contracts/GeneralVault.sol#L144-L148

13. Consider two-phase ownership transfer

Consider adding a two-phase transfer, where the current owner nominates the next owner, and the next owner has to call accept*() to become the new owner. This prevents passing the ownership to an account that is unable to use it.

There is 1 instance of this issue:

File: smart-contracts/YieldManager.sol   #1

26:  contract YieldManager is VersionedInitializable, Ownable {

https://github.com/code-423n4/2022-05-sturdy/blob/78f51a7a74ebe8adfd055bdbaedfddc05632566f/smart-contracts/YieldManager.sol#L26

sforman2000 commented 2 years ago

Particularly high quality.

atozICT20 commented 2 years ago

Issue	Comments
L1	Fixed
L2	Fixed
L3	Fixed
L4	Fixed
L5	Invalid. ETH constant may be used in several child contract.
L6	Admin can monitor it.
L7	Invalid. LidoVault can be received ETH from CurveSwap
L8	No need to change
L9	Fixed
L10	Fixed
N1	Fixed
N2	Fixed
N3	Invalid
N4	Fixed
N5	Fixed
N6	Fixed
N7	Fixed
N8	Fixed
N9	No need to change
N10	Invalid
N11	Invalid
N12	Fixed
N13	No need to change

HickupHH3 commented 2 years ago

Bumping L1 as a high-severity duplicate of #133. I'm being lenient here as one might argue that the critical step of linking the slippage check of GeneralVault to causing stuck user fund withdrawals was not made. It just happens to be that the LIDO vault was an exception, where users can withdraw directly in stETH, as the warden has reasoned, so I will give the warden the benefit of the doubt this time.

L5 should be addressed IMO. there is an inconsistency between addresses being used. The LIDO vault should be using the ETH constant instead of the null address for ETH, or vice versa. For someone who uses etherscan, he'll see the ETH constant define and assumes that he should be using that to specify ETH, then wonder why his tx will potentially revert in Metamask.

Low issues: L2, L3, L4, L5, L6, L8, centralisation risk NC issues: L9, L10, N1, N2, N3 (more of sponsor acknowledged), N4, N5, N6, N7 (reasoning is diff from N6), N8, N9, N12, N13 Invalid: L7, N10 (some fields are not worth the extra gas to index), N11 (no justification),

code-423n4 / 2022-05-sturdy-findings

QA Report #69

Summary

Low Risk Issues

Non-critical Issues

Low Risk Issues

1. Slippage of 1% is too strict

2. Mistaken null values cause `distributeYield()` to revert

3. Can't remove old assets

4. Missing checks for `approve()`'s return status

5. Move `ETH` constant to child contract

6. Unsafe casts and usage of `IERC20Detailed`

7. Unused `receive()` function will lock Ether in contract

8. `safeApprove()` is deprecated

9. Missing checks for `address(0x0)` when assigning values to `address` state variables

10. Open TODOs

Non-critical Issues

1. `override` function arguments that are unused should have the variable name removed or commented out to avoid compiler warnings

2. `public` functions not called by the contract should be declared `external` instead

3. `constant`s should be defined rather than using magic numbers

4. Redundant cast

5. Missing event for critical parameter change

6. Use a more recent version of solidity

7. Use a more recent version of solidity

8. Variable names that consist of all capital letters should be reserved for `const`/`immutable` variables

9. NatSpec is incomplete

10. Event is missing `indexed` fields

11. Consider allowing the passing of a referral code

12. Remove commented out code

13. Consider two-phase ownership transfer

code-423n4 / 2022-05-sturdy-findings

QA Report #69

Summary

Low Risk Issues

Non-critical Issues

Low Risk Issues

1. Slippage of 1% is too strict

2. Mistaken null values cause distributeYield() to revert

3. Can't remove old assets

4. Missing checks for approve()'s return status

5. Move ETH constant to child contract

6. Unsafe casts and usage of IERC20Detailed

7. Unused receive() function will lock Ether in contract

8. safeApprove() is deprecated

9. Missing checks for address(0x0) when assigning values to address state variables

10. Open TODOs

Non-critical Issues

1. override function arguments that are unused should have the variable name removed or commented out to avoid compiler warnings

2. public functions not called by the contract should be declared external instead

3. constants should be defined rather than using magic numbers

4. Redundant cast

5. Missing event for critical parameter change

6. Use a more recent version of solidity

7. Use a more recent version of solidity

8. Variable names that consist of all capital letters should be reserved for const/immutable variables

9. NatSpec is incomplete

10. Event is missing indexed fields

11. Consider allowing the passing of a referral code

12. Remove commented out code

13. Consider two-phase ownership transfer

2. Mistaken null values cause `distributeYield()` to revert

4. Missing checks for `approve()`'s return status

5. Move `ETH` constant to child contract

6. Unsafe casts and usage of `IERC20Detailed`

7. Unused `receive()` function will lock Ether in contract

8. `safeApprove()` is deprecated

9. Missing checks for `address(0x0)` when assigning values to `address` state variables

1. `override` function arguments that are unused should have the variable name removed or commented out to avoid compiler warnings

2. `public` functions not called by the contract should be declared `external` instead

3. `constant`s should be defined rather than using magic numbers

8. Variable names that consist of all capital letters should be reserved for `const`/`immutable` variables

10. Event is missing `indexed` fields