Arbitrary Code Execution in LRTDepositPool.depositAsset.

[c4-submissions]

Lines of code

https://github.com/code-423n4/2023-11-kelp/blob/4b34abc952205e2a34bff893a0de0c75b8052149/src/LRTDepositPool.sol#L116-L144

Vulnerability details

Impact

depositAsset function allows an arbitrary address to be passed in as the asset parameter. This opens up the contract to be exploited if a malicious contract address is passed in.

Specifically, the problem lies in this line:

if (!IERC20(asset).transferFrom(msg.sender, address(this), depositAmount)) {
  revert TokenTransferFailed(); 
}

Here transferFrom is called on the address passed as asset. If a malicious contract is passed in, its transferFrom function could execute arbitrary logic, which could steal funds, manipulate state, etc.

This is possible because:

• Solidity allows to call functions on any address, regardless of whether it contains code or not. So, calling IERC20(asset).transferFrom will attempt to execute the transferFrom function in the contract deployed at asset.

• There are no checks on asset to ensure it is a valid token contract address and not a malicious contract.

• The IERC20 interface declaration alone doesn't provide any protections. The calling contract still attempts to execute whatever code is present at that address.

Proof of Concept

The depositAsset function in the LRTDepositPool.

    /// @notice helps user stake LST to the protocol
    /// @param asset LST asset address to stake
    /// @param depositAmount LST asset amount to stake
    function depositAsset(
        address asset,
        uint256 depositAmount
    )
        external
        whenNotPaused
        nonReentrant
        onlySupportedAsset(asset)
    {
        // checks
        if (depositAmount == 0) {
            revert InvalidAmount();
        }
        if (depositAmount > getAssetCurrentLimit(asset)) {
            revert MaximumDepositLimitReached();
        }

        if (!IERC20(asset).transferFrom(msg.sender, address(this), depositAmount)) {
            revert TokenTransferFailed();
        }

        // interactions
        uint256 rsethAmountMinted = _mintRsETH(asset, depositAmount);

        emit AssetDeposit(asset, depositAmount, rsethAmountMinted);
    }

This is dangerous because it trusts the asset address to be a valid ERC20 token contract. However, an attacker could provide a malicious contract address that executes arbitrary code in its transferFrom function. There is no validation on the asset parameter before calling transferFrom on it. This allows a malicious contract address to be provided.

// LRTDepositPool.sol

function depositAsset(address asset, uint amount) external {

  IERC20(asset).transferFrom(msg.sender, address(this), amount);

}

This is dangerous because it trusts the asset address to be a valid ERC20 token contract. However, an attacker could provide a malicious contract address that executes arbitrary code in its transferFrom function.

If exploited, this would enable an attacker to execute arbitrary code inside transferFrom. This could allow them to:

• Drain funds from the contract by calling transfer or send
• Corrupt contract state by manipulating storage
• Trigger further exploits by invoking other contracts

Attackers could steal potentially unlimited funds that are approved to the LRTDepositPool contract.

Exploitation

An example exploit:

1. Attacker deploys MaliciousToken contract
2. User approves LRTDepositPool to transfer 100 tokens
3. Attacker calls depositAsset with MaliciousToken address
4. transferFrom in MaliciousToken is executed
5. transferFrom drains funds to the attacker

Tools Used

Manual review

Recommended Mitigation Steps

• Add a modifier that checks asset is not a contract: For example.

modifier notContract(address addr) {
  require(!addr.isContract(), "No contracts");
  _;
}

function depositAsset(
  address asset, 
  uint256 amount
) external notContract(asset) {
  // ...
}

• Use OpenZeppelin's Address.isContract to check in depositAsset:

function depositAsset(
  address asset,
  uint256 amount  
) external {

  require(!Address.isContract(asset), "No contracts");

  // ...
}

• Maintain a whitelist of allowed token addresses that are manually checked to not be contracts before adding to the list.

• Use a low-level call instead of transferFrom so a malicious transferFrom can't be executed:

(bool success, ) = asset.call(abi.encodeWithSignature("transferFrom(address,address,uint256)", msg.sender, address(this), amount));
require(success, "Transfer failed");

The key is to validate asset is not a malicious contract before calling any of its functions. Checking !Address.isContract(asset) or maintaining a whitelist are safer ways to do this.

Assessed type

ETH-Transfer

[c4-pre-sort]

raymondfam marked the issue as insufficient quality report

[c4-pre-sort]

raymondfam marked the issue as primary issue

[raymondfam]

The user will be minted 0 RSETH if the call ever succeeds atomically because lrtOracle.getAssetPrice(asset) will be 0 for a non-existent/supported asset:

https://github.com/code-423n4/2023-11-kelp/blob/4b34abc952205e2a34bff893a0de0c75b8052149/src/LRTDepositPool.sol#L109

        rsethAmountToMint = (amount * lrtOracle.getAssetPrice(asset)) / lrtOracle.getRSETHPrice();

Moreover, it's being denied by the modifier onlySupportedAsset(asset).

[c4-judge]

fatherGoose1 marked the issue as unsatisfactory: Invalid