Stader Labs - jaraxxus's results

Decentralized ETH liquid staking protocol with 4 ETH bond for anyone to be a node operator.

General Information

Platform: Code4rena

Start Date: 02/06/2023

Pot Size: $100,000 USDC

Total HM: 15

Participants: 75

Period: 7 days

Judge: Picodes

Total Solo HM: 5

Id: 249

League: ETH

Stader Labs

Findings Distribution

Researcher Performance

Rank: 32/75

Findings: 2

Award: $120.84

QA:

grade-b

🌟 Selected for report: 0

🚀 Solo Findings: 0

External Links

Code4rena Contest Page Github Contest Repository Github Findings Repository Stader Labs

Findings Information

🌟 Selected for report: Aymen0909

Also found by: ChrisTina, Deps, LaScaloneta, NoamYakov, SovaSlava, T1MOH, bin2chen, djxploit, jaraxxus, josephdara, martin, tallo

Awards

102.2712 USDC - $102.27

Labels

bug

2 (Med Risk)

satisfactory

duplicate-383

External Links

Link to GitHub issue

Lines of code

https://github.com/code-423n4/2023-06-stader/blob/7566b5a35f32ebd55d3578b8bd05c038feb7d9cc/contracts/Auction.sol#L11

Vulnerability details

Impact

Auction.sol cannot use pause functions but have whenNotPaused modifier.

Proof of Concept

Contract Auction.sol uses PausableUpgradeable but does not have exposed pause() function like in other contracts

contract Auction is IAuction, Initializable, AccessControlUpgradeable, PausableUpgradeable, ReentrancyGuardUpgradeable {
    IStaderConfig public override staderConfig;
    uint256 public override nextLot;
    uint256 public override bidIncrement;
    uint256 public override duration;

    mapping(uint256 => LotItem) public lots;

    uint256 public constant MIN_AUCTION_DURATION = 7200; // 24 hours

    /// @custom:oz-upgrades-unsafe-allow constructor
    constructor() {
        _disableInitializers();
    }

    function initialize(address _admin, address _staderConfig) external initializer {
        UtilLib.checkNonZeroAddress(_admin);
        UtilLib.checkNonZeroAddress(_staderConfig);

        __AccessControl_init();
        __Pausable_init();

Tools Used

Manual Review

Recommended Mitigation Steps

Expose pause() function like how it is done in other contracts like StaderStakePoolsManager

    function pause() external onlyRole(DEFAULT_ADMIN_ROLE) {
        _pause();
    }

    /**
     * @dev Returns to normal state.
     * Contract must be paused
     */
    function unpause() external onlyRole(DEFAULT_ADMIN_ROLE) {
        _unpause();
    }

Assessed type

Upgradable

#0 - c4-judge
2023-06-10T10:45:59Z

Picodes marked the issue as duplicate of #383

#1 - c4-judge
2023-07-02T09:44:17Z

Picodes marked the issue as satisfactory

Findings Information

🌟 Selected for report: RaymondFam

Also found by: 0xWaitress, 0xhacksmithh, ChrisTina, DadeKuma, LaScaloneta, Rolezn, SAAJ, Sathish9098, T1MOH, bin2chen, btk, catellatech, ernestognw, fatherOfBlocks, hals, hunter_w3b, jaraxxus, matrix_0wl, mgf15, naman1778, niser93, solsaver, turvy_fuzz

Awards

18.5651 USDC - $18.57

Labels

bug

grade-b

QA (Quality Assurance)

Q-21

External Links

Link to GitHub issue

Low Issues

	Issue	Instances
L-1	`abi.encodePacked()` should not be used with dynamic types when passing the result to a hash function such as `keccak256()`	1
L-2	Empty Function Body - Consider commenting why	3
L-3	Initializers could be front-run	64
L-4	Unsafe ERC20 operation(s)	7
NC-1	Return values of `approve()` not checked	1
NC-2	Functions not used internally could be marked external	27

[L-1] `abi.encodePacked()` should not be used with dynamic types when passing the result to a hash function such as `keccak256()`

Use abi.encode() instead which will pad items to 32 bytes, which will prevent hash collisions (e.g. abi.encodePacked(0x123,0x456) => 0x123456 => abi.encodePacked(0x1,0x23456), but abi.encode(0x123,0x456) => 0x0...1230...456). "Unless there is a compelling reason, abi.encode should be preferred". If there is only one argument to abi.encodePacked() it can often be cast to bytes() or bytes32() instead. If all arguments are strings and or bytes, bytes.concat() should be used instead

Instances (1):

File: example/SocializingPool.sol

162:         bytes32 node = keccak256(abi.encodePacked(_operator, _amountSD, _amountETH));

[L-2] Empty Function Body - Consider commenting why

Instances (3):

File: example/NodeELRewardVault.sol

5:     constructor() {}

File: example/ValidatorWithdrawalVault.sol

9:     constructor() {}

File: example/VaultProxy.sol

14:     constructor() {}

[L-3] Initializers could be front-run

Initializers could be front-run, allowing an attacker to either set their own values, take ownership of the contract, and in the best case forcing a re-deployment

Instances (64):

File: example/Auction.sol

19:     function initialize(address _admin, address _staderConfig) external initializer {

19:     function initialize(address _admin, address _staderConfig) external initializer {

23:         __AccessControl_init();

24:         __Pausable_init();

25:         __ReentrancyGuard_init();

File: example/ETHx.sol

22:     function initialize(address _admin, address _staderConfig) public initializer {

22:     function initialize(address _admin, address _staderConfig) public initializer {

26:         __ERC20_init('ETHx', 'ETHx');

27:         __Pausable_init();

28:         __AccessControl_init();

File: example/OperatorRewardsCollector.sol

19:     function initialize(address _admin, address _staderConfig) external initializer {

19:     function initialize(address _admin, address _staderConfig) external initializer {

23:         __AccessControl_init();

24:         __Pausable_init();

File: example/Penalty.sol

21:     function initialize(

25:     ) external initializer {

30:         __ReentrancyGuard_init();

File: example/PermissionedNodeRegistry.sol

50:     function initialize(address _admin, address _staderConfig) public initializer {

50:     function initialize(address _admin, address _staderConfig) public initializer {

54:         __Pausable_init();

55:         __ReentrancyGuard_init();

File: example/PermissionedPool.sol

23:     function initialize(address _admin, address _staderConfig) public initializer {

23:     function initialize(address _admin, address _staderConfig) public initializer {

27:         __ReentrancyGuard_init();

File: example/PermissionlessNodeRegistry.sol

48:     function initialize(address _admin, address _staderConfig) public initializer {

48:     function initialize(address _admin, address _staderConfig) public initializer {

52:         __Pausable_init();

53:         __ReentrancyGuard_init();

File: example/PermissionlessPool.sol

24:     function initialize(address _admin, address _staderConfig) public initializer {

24:     function initialize(address _admin, address _staderConfig) public initializer {

28:         __ReentrancyGuard_init();

File: example/PoolSelector.sol

27:     function initialize(address _admin, address _staderConfig) external initializer {

27:     function initialize(address _admin, address _staderConfig) external initializer {

File: example/PoolUtils.sol

17:     function initialize(address _admin, address _staderConfig) public initializer {

17:     function initialize(address _admin, address _staderConfig) public initializer {

File: example/SDCollateral.sol

14:     function initialize(address _admin, address _staderConfig) public initializer {

14:     function initialize(address _admin, address _staderConfig) public initializer {

18:         __AccessControl_init();

19:         __ReentrancyGuard_init();

File: example/SocializingPool.sol

27:     function initialize(address _admin, address _staderConfig) public initializer {

27:     function initialize(address _admin, address _staderConfig) public initializer {

31:         __AccessControl_init();

32:         __Pausable_init();

33:         __ReentrancyGuard_init();

File: example/StaderConfig.sol

79:     function initialize(address _admin, address _ethDepositContract) external initializer {

79:     function initialize(address _admin, address _ethDepositContract) external initializer {

82:         __AccessControl_init();

File: example/StaderInsuranceFund.sol

18:     function initialize(address _admin, address _staderConfig) public initializer {

18:     function initialize(address _admin, address _staderConfig) public initializer {

22:         __ReentrancyGuard_init();

File: example/StaderOracle.sol

49:     function initialize(address _admin, address _staderConfig) public initializer {

49:     function initialize(address _admin, address _staderConfig) public initializer {

53:         __AccessControl_init();

54:         __Pausable_init();

55:         __ReentrancyGuard_init();

File: example/StaderStakePoolsManager.sol

34:     function initialize(address _admin, address _staderConfig) public initializer {

34:     function initialize(address _admin, address _staderConfig) public initializer {

37:         __AccessControl_init();

38:         __Pausable_init();

39:         __ReentrancyGuard_init();

File: example/UserWithdrawalManager.sol

40:     function initialize(address _admin, address _staderConfig) public initializer {

40:     function initialize(address _admin, address _staderConfig) public initializer {

44:         __Pausable_init();

45:         __ReentrancyGuard_init();

[L-4] Unsafe ERC20 operation(s)

Instances (7):

File: example/Auction.sol

45:         if (!IERC20(staderConfig.getStaderToken()).transferFrom(msg.sender, address(this), _sdAmount)) {

77:         if (!IERC20(staderConfig.getStaderToken()).transfer(lotItem.highestBidder, lotItem.sdAmount)) {

104:         if (!IERC20(staderConfig.getStaderToken()).transfer(staderConfig.getStaderTreasury(), _sdAmount)) {

File: example/SDCollateral.sol

35:         if (!IERC20(staderConfig.getStaderToken()).transferFrom(operator, address(this), _sdAmount)) {

56:         if (!IERC20(staderConfig.getStaderToken()).transfer(payable(operator), _requestedSD)) {

94:         IERC20(staderConfig.getStaderToken()).approve(auctionContract, type(uint256).max);

File: example/SocializingPool.sol

117:             if (!IERC20(staderConfig.getStaderToken()).transfer(operatorRewardsAddr, totalAmountSD)) {

[NC-1] Return values of `approve()` not checked

Not all IERC20 implementations revert() when there's a failure in approve(). The function signature has a boolean return value and they indicate errors that way instead. By not checking the return value, operations that should have marked as failed, may potentially go through without actually approving anything

Instances (1):

File: example/SDCollateral.sol

94:         IERC20(staderConfig.getStaderToken()).approve(auctionContract, type(uint256).max);

[NC-2] Functions not used internally could be marked external

Instances (27):

File: example/ETHx.sol

22:     function initialize(address _admin, address _staderConfig) public initializer {

File: example/PermissionedNodeRegistry.sol

50:     function initialize(address _admin, address _staderConfig) public initializer {

File: example/PermissionedPool.sol

23:     function initialize(address _admin, address _staderConfig) public initializer {

File: example/PermissionlessNodeRegistry.sol

48:     function initialize(address _admin, address _staderConfig) public initializer {

422:     function getTotalQueuedValidatorCount() public view override returns (uint256) {

File: example/PermissionlessPool.sol

24:     function initialize(address _admin, address _staderConfig) public initializer {

File: example/PoolUtils.sol

17:     function initialize(address _admin, address _staderConfig) public initializer {

128:     function getSocializingPoolAddress(uint8 _poolId)

139:     function getOperatorTotalNonTerminalKeys(

File: example/SDCollateral.sol

14:     function initialize(address _admin, address _staderConfig) public initializer {

193:     function convertSDToETH(uint256 _sdAmount) public view override returns (uint256) {

File: example/SocializingPool.sol

27:     function initialize(address _admin, address _staderConfig) public initializer {

File: example/StaderConfig.sol

498:     function onlyManagerRole(address account) public view override returns (bool) {

502:     function onlyOperatorRole(address account) public view override returns (bool) {

File: example/StaderInsuranceFund.sol

18:     function initialize(address _admin, address _staderConfig) public initializer {

File: example/StaderOracle.sol

49:     function initialize(address _admin, address _staderConfig) public initializer {

558:     function getERReportableBlock() public view override returns (uint256) {

569:     function getSDPriceReportableBlock() public view override returns (uint256) {

573:     function getValidatorStatsReportableBlock() public view override returns (uint256) {

577:     function getWithdrawnValidatorReportableBlock() public view override returns (uint256) {

File: example/StaderStakePoolsManager.sol

34:     function initialize(address _admin, address _staderConfig) public initializer {

109:     function getExchangeRate() public view override returns (uint256) {

124:     function convertToShares(uint256 _assets) public view override returns (uint256) {

129:     function convertToAssets(uint256 _shares) public view override returns (uint256) {

148:     function previewWithdraw(uint256 _shares) public view override returns (uint256) {

153:     function deposit(address _receiver) public payable override whenNotPaused returns (uint256) {

File: example/UserWithdrawalManager.sol

40:     function initialize(address _admin, address _staderConfig) public initializer {

#0 - c4-judge
2023-06-14T06:49:43Z

Picodes marked the issue as grade-b

Stader Labs - jaraxxus's results

Decentralized ETH liquid staking protocol with 4 ETH bond for anyone to be a node operator.

General Information

Findings Distribution

Researcher Performance

External Links

[M-02] contract Auction.sol does not have pause function exposed - $102.27

Findings Information

Awards

Labels

External Links

Lines of code

Vulnerability details

Impact

Proof of Concept

Tools Used

Recommended Mitigation Steps

Assessed type

#0 - c4-judge2023-06-10T10:45:59Z

#1 - c4-judge2023-07-02T09:44:17Z

[Q-21] QA Report - $18.57

Findings Information

Awards

Labels

External Links

Low Issues

[L-1] abi.encodePacked() should not be used with dynamic types when passing the result to a hash function such as keccak256()

[L-2] Empty Function Body - Consider commenting why

[L-3] Initializers could be front-run

[L-4] Unsafe ERC20 operation(s)

[NC-1] Return values of approve() not checked

[NC-2] Functions not used internally could be marked external

#0 - c4-judge2023-06-14T06:49:43Z

#0 - c4-judge
2023-06-10T10:45:59Z

#1 - c4-judge
2023-07-02T09:44:17Z

[L-1] `abi.encodePacked()` should not be used with dynamic types when passing the result to a hash function such as `keccak256()`

[NC-1] Return values of `approve()` not checked

#0 - c4-judge
2023-06-14T06:49:43Z