Kelp DAO | rsETH - Draiakoo's results

Lines of code

https://github.com/code-423n4/2023-11-kelp/blob/main/src/LRTDepositPool.sol#L162-L176 https://github.com/code-423n4/2023-11-kelp/blob/main/src/LRTDepositPool.sol#L119-L144

Vulnerability details

Impact

Medium impact It does not have big chances to happen and if it happens it can be solved by upgrading the LRTDepositPool contract. However, if it happens, nobody would be able to deposit assets into the protocol until the deposit pool is upgraded.

Proof of Concept

If a contract that does not implement the function getAssetBalance(address) is registered as a node delegator in LRTDepositPool, nobody will be able to deposit assets until the contract gets upgraded, because there is no function to modify or remove an address inside the nodeDelegatorQueue array. Foundry POC:

// SPDX-License-Identifier: UNLICENSED

pragma solidity 0.8.21;

import { Test } from "forge-std/Test.sol";
import { ERC20 } from "@openzeppelin/contracts/token/ERC20/ERC20.sol";
import { TransparentUpgradeableProxy } from "@openzeppelin/contracts/proxy/transparent/TransparentUpgradeableProxy.sol";
import { ProxyAdmin } from "@openzeppelin/contracts/proxy/transparent/ProxyAdmin.sol";
import { LRTConfig, ILRTConfig } from "src/LRTConfig.sol";
import { RSETH } from "src/RSETH.sol";
import { LRTDepositPool } from "src/LRTDepositPool.sol";

import { LRTConstants } from "src/utils/LRTConstants.sol";
import { PausableUpgradeable } from "@openzeppelin/contracts-upgradeable/security/PausableUpgradeable.sol";

contract MockToken is ERC20 {
    constructor(string memory name, string memory symbol) ERC20(name, symbol) { }

    function mint(address to, uint256 amount) external {
        _mint(to, amount);
    }
}

contract LRTOracleMock {

    LRTConfig public lrtConfig;
    mapping(address asset => uint256 price) public prices;

    constructor(address lrtConfigAddr) {
        lrtConfig = LRTConfig(lrtConfigAddr);
    }

    function setAssetPrice(address asset, uint256 amount) external {
        prices[asset] = amount;
    }

    function getAssetPrice(address asset) public view returns (uint256) {
        return prices[asset];
    }

    function getRSETHPrice() external view returns (uint256 rsETHPrice) {
        address rsETHTokenAddress = lrtConfig.rsETH();
        uint256 rsEthSupply = RSETH(rsETHTokenAddress).totalSupply();

        if (rsEthSupply == 0) {
            return 1 ether;
        }

        uint256 totalETHInPool;
        address lrtDepositPoolAddr = lrtConfig.getContract(LRTConstants.LRT_DEPOSIT_POOL);

        address[] memory supportedAssets = lrtConfig.getSupportedAssetList();
        uint256 supportedAssetCount = supportedAssets.length;

        for (uint16 asset_idx; asset_idx < supportedAssetCount;) {
            address asset = supportedAssets[asset_idx];
            uint256 assetER = getAssetPrice(asset);

            uint256 totalAssetAmt = LRTDepositPool(lrtDepositPoolAddr).getTotalAssetDeposits(asset);
            totalETHInPool += totalAssetAmt * assetER;

            unchecked {
                ++asset_idx;
            }
        }

        return totalETHInPool / rsEthSupply;
    }
}

contract AuditTests is Test {
    MockToken public stETH;
    MockToken public rETH;
    MockToken public cbETH;
    LRTConfig public lrtConfig;
    RSETH public rseth;
    LRTDepositPool public lrtDepositPool;
    LRTOracleMock public lrtOracle;
    address public manager = makeAddr("manager");

    address public admin = makeAddr("admin");

    address public alice = makeAddr("alice");
    address public bob = makeAddr("bob");
    address public carol = makeAddr("carol");

    function setUp() public virtual {
        stETH = new MockToken("staked ETH", "stETH");
        rETH = new MockToken("rETH", "rETH");
        cbETH = new MockToken("cbETH", "cbETH");

        ProxyAdmin proxyAdmin = new ProxyAdmin();
        LRTConfig lrtConfigImpl = new LRTConfig();
        TransparentUpgradeableProxy lrtConfigProxy = new TransparentUpgradeableProxy(
            address(lrtConfigImpl),
            address(proxyAdmin),
            ""
        );

        RSETH tokenImpl = new RSETH();
        TransparentUpgradeableProxy tokenProxy = new TransparentUpgradeableProxy(
            address(tokenImpl),
            address(proxyAdmin),
            ""
        );

        LRTDepositPool contractImpl = new LRTDepositPool();
        TransparentUpgradeableProxy contractProxy = new TransparentUpgradeableProxy(
            address(contractImpl),
            address(proxyAdmin),
            ""
        );

        lrtConfig = LRTConfig(address(lrtConfigProxy));
        lrtConfig.initialize(admin, address(stETH), address(rETH), address(cbETH), address(1));

        lrtOracle = new LRTOracleMock(address(lrtConfig));

        lrtDepositPool = LRTDepositPool(address(contractProxy));
        lrtDepositPool.initialize(address(lrtConfig));

        rseth = RSETH(address(tokenProxy));
        rseth.initialize(address(admin), address(lrtConfig));

        vm.startPrank(admin);
        lrtConfig.grantRole(LRTConstants.MANAGER, manager);
        lrtConfig.setContract(LRTConstants.LRT_ORACLE, address(lrtOracle));
        lrtConfig.setContract(LRTConstants.LRT_DEPOSIT_POOL, address(lrtDepositPool));
        lrtConfig.setRSETH(address(rseth));
        rseth.grantRole(rseth.MINTER_ROLE(), address(lrtDepositPool));
        vm.stopPrank();

        // set prices for tokens
        lrtOracle.setAssetPrice(address(stETH), 1 ether);      // 1 ETH = 1 stETH
        lrtOracle.setAssetPrice(address(rETH), 2 ether);       // 2 ETH = 1 rETH
        lrtOracle.setAssetPrice(address(cbETH), 3 ether);      // 3 ETH = 1 cbETH
    }

    function test_WrongNodeDelegatorRegistered() public {

        vm.startPrank(admin);
        WrongNodeDelegatorImplementation wrongNodeDelegator = new WrongNodeDelegatorImplementation();
        address[] memory newNodes = new address[](1);
        newNodes[0] = address(wrongNodeDelegator);
        lrtDepositPool.addNodeDelegatorContractToQueue(newNodes);

        stETH.mint(alice, 1 ether);

        // Now nobody can depositAssets into the protocol
        vm.startPrank(alice);
        stETH.approve(address(lrtDepositPool), 1 ether);
        vm.expectRevert();
        lrtDepositPool.depositAsset(address(stETH), 1 ether);
        vm.stopPrank();
    }
}

contract WrongNodeDelegatorImplementation {
    // It does not contain getAssetBalance(address)
}

Traces

Running 1 test for test/BaseTest.t.sol:AuditTests
[PASS] test_WrongNodeDelegatorRegistered() (gas: 227024)
Traces:
  [227024] AuditTests::test_WrongNodeDelegatorRegistered()
    ├─ [0] VM::startPrank(admin: [0xaA10a84CE7d9AE517a52c6d5cA153b369Af99ecF])
    │   └─ ← ()
    ├─ [12464] → new WrongNodeDelegatorImplementation@0x3Ede3eCa2a72B3aeCC820E955B36f38437D01395
    │   └─ ← 62 bytes of code
    ├─ [70787] TransparentUpgradeableProxy::addNodeDelegatorContractToQueue([0x3Ede3eCa2a72B3aeCC820E955B36f38437D01395])
    │   ├─ [63686] LRTDepositPool::addNodeDelegatorContractToQueue([0x3Ede3eCa2a72B3aeCC820E955B36f38437D01395]) [delegatecall]
    │   │   ├─ [9792] TransparentUpgradeableProxy::hasRole(0x0000000000000000000000000000000000000000000000000000000000000000, admin: [0xaA10a84CE7d9AE517a52c6d5cA153b369Af99ecF]) [staticcall]
    │   │   │   ├─ [2694] LRTConfig::hasRole(0x0000000000000000000000000000000000000000000000000000000000000000, admin: [0xaA10a84CE7d9AE517a52c6d5cA153b369Af99ecF]) [delegatecall]
    │   │   │   │   └─ ← true
    │   │   │   └─ ← true
    │   │   ├─ emit NodeDelegatorAddedinQueue(prospectiveNodeDelegatorContract: WrongNodeDelegatorImplementation: [0x3Ede3eCa2a72B3aeCC820E955B36f38437D01395])
    │   │   └─ ← ()
    │   └─ ← ()
    ├─ [46588] MockToken::mint(alice: [0x328809Bc894f92807417D2dAD6b7C998c1aFdac6], 1000000000000000000 [1e18])
    │   ├─ emit Transfer(from: 0x0000000000000000000000000000000000000000, to: alice: [0x328809Bc894f92807417D2dAD6b7C998c1aFdac6], value: 1000000000000000000 [1e18])
    │   └─ ← ()
    ├─ [0] VM::startPrank(alice: [0x328809Bc894f92807417D2dAD6b7C998c1aFdac6])
    │   └─ ← ()
    ├─ [24599] MockToken::approve(TransparentUpgradeableProxy: [0xD6BbDE9174b1CdAa358d2Cf4D57D1a9F7178FBfF], 1000000000000000000 [1e18])
    │   ├─ emit Approval(owner: alice: [0x328809Bc894f92807417D2dAD6b7C998c1aFdac6], spender: TransparentUpgradeableProxy: [0xD6BbDE9174b1CdAa358d2Cf4D57D1a9F7178FBfF], value: 1000000000000000000 [1e18])
    │   └─ ← true
    ├─ [0] VM::expectRevert()
    │   └─ ← ()
    ├─ [20479] TransparentUpgradeableProxy::depositAsset(MockToken: [0x5615dEB798BB3E4dFa0139dFa1b3D433Cc23b72f], 1000000000000000000 [1e18])
    │   ├─ [19883] LRTDepositPool::depositAsset(MockToken: [0x5615dEB798BB3E4dFa0139dFa1b3D433Cc23b72f], 1000000000000000000 [1e18]) [delegatecall]
    │   │   ├─ [3156] TransparentUpgradeableProxy::isSupportedAsset(MockToken: [0x5615dEB798BB3E4dFa0139dFa1b3D433Cc23b72f]) [staticcall]
    │   │   │   ├─ [2561] LRTConfig::isSupportedAsset(MockToken: [0x5615dEB798BB3E4dFa0139dFa1b3D433Cc23b72f]) [delegatecall]
    │   │   │   │   └─ ← true
    │   │   │   └─ ← true
    │   │   ├─ [1156] TransparentUpgradeableProxy::isSupportedAsset(MockToken: [0x5615dEB798BB3E4dFa0139dFa1b3D433Cc23b72f]) [staticcall]
    │   │   │   ├─ [561] LRTConfig::isSupportedAsset(MockToken: [0x5615dEB798BB3E4dFa0139dFa1b3D433Cc23b72f]) [delegatecall]
    │   │   │   │   └─ ← true
    │   │   │   └─ ← true
    │   │   ├─ [2558] MockToken::balanceOf(TransparentUpgradeableProxy: [0xD6BbDE9174b1CdAa358d2Cf4D57D1a9F7178FBfF]) [staticcall]
    │   │   │   └─ ← 0
    │   │   ├─ [2558] MockToken::balanceOf(WrongNodeDelegatorImplementation: [0x3Ede3eCa2a72B3aeCC820E955B36f38437D01395]) [staticcall]
    │   │   │   └─ ← 0
    │   │   ├─ [23] WrongNodeDelegatorImplementation::getAssetBalance(MockToken: [0x5615dEB798BB3E4dFa0139dFa1b3D433Cc23b72f]) [staticcall]
    │   │   │   └─ ← "EvmError: Revert"
    │   │   └─ ← "EvmError: Revert"
    │   └─ ← "EvmError: Revert"
    ├─ [0] VM::stopPrank()
    │   └─ ← ()
    └─ ← ()

Test result: ok. 1 passed; 0 failed; finished in 12.60ms

Tools Used

Manual review

Recommended Mitigation Steps

Implement a node delegator validation when registering new nodes. Node delegator contracts would need to have this function:

    function isNodeDelegator() external pure returns(bool){
        return true;
    }

And the function to register new node delegators would be as follows:

    function addNodeDelegatorContractToQueue(address[] calldata nodeDelegatorContracts) external onlyLRTAdmin {
        uint256 length = nodeDelegatorContracts.length;
        if (nodeDelegatorQueue.length + length > maxNodeDelegatorCount) {
            revert MaximumNodeDelegatorCountReached();
        }

        for (uint256 i; i < length;) {
            UtilLib.checkNonZeroAddress(nodeDelegatorContracts[i]);
            require(INodeDelegator(nodeDelegatorContracts[i]).isNodeDelegator(), "wrong node");
            nodeDelegatorQueue.push(nodeDelegatorContracts[i]);
            emit NodeDelegatorAddedinQueue(nodeDelegatorContracts[i]);
            unchecked {
                ++i;
            }
        }
    }

Assessed type

Invalid Validation