c4-submissions commented 10 months ago

Lines of code

https://github.com/code-423n4/2023-11-kelp/blob/c5fdc2e62c5e1d78769f44d6e34a6fb9e40c00f0/src/LRTConfig.sol#L70-L76 https://github.com/code-423n4/2023-11-kelp/blob/c5fdc2e62c5e1d78769f44d6e34a6fb9e40c00f0/src/LRTConfig.sol#L70-L76

Vulnerability details

Impact

The addNewSupportedAsset function does not validate asset addresses. This allows anyone to add malicious or invalid contracts as supported assets. These could manipulate protocol behavior or steal funds when interacted with.

Proof of Concept

addNewSupportedAsset takes an asset address as input. But does not verify that this address is a valid ERC20 token contract.

This means an arbitrary address could be added as a "supported" asset, not just real tokens.

Could add scam or malicious contracts as approved assets that steal funds or manipulate protocol behavior.
Integrations with the listed assets may fail or act unpredictably if they are not actual tokens.
Hurts integrity of asset whitelist, since it could contain non-token contracts.
UI/tooling may display invalid assets as tradeable options.

The addNewSupportedAsset external function which:

Takes an asset address and deposit limit as parameters
Checks the MANAGER role using the OpenZeppelin onlyRole modifier
Calls the internal _addNewSupportedAsset function to add the asset

The key issue is that addNewSupportedAsset does not verify that the asset address passed in is a valid ERC20 token contract. It only relies on the access control check from onlyRole.

So any address could be passed and added as a supported asset, even if it is not an actual token contract.

addNewSupportedAsset function

function addNewSupportedAsset(address asset, uint256 depositLimit) external {
  // No input validation on `asset` 
  _addNewSupportedAsset(asset, depositLimit);
}

This allows adding any arbitrary address as a supported asset.

An attacker could:

Deploy a malicious token contract:

contract MalToken {
  function transferFrom(address, address, uint) external {
    // Drain funds
  }
}

Get it added as a supported asset:

LRTConfig(config).addNewSupportedAsset(malToken, 100);

When transfers are attempted on the fake token, it drains funds:

IERC20(malToken).transferFrom(user, pool, 10); // Calls malicious contract

This test demonstrates adding a malicious contract, and it draining funds when used as a "supported" asset.

// test/LRTConfig.t.sol

import "forge-std/Test.sol";
import {LRTConfig} from "../src/LRTConfig.sol";

contract Exploit is Test {
  LRTConfig config;
  MalToken malToken;

  function testAddMaliciousAsset() public {

    malToken = new MalToken(); // malicious contract

    config.addNewSupportedAsset(address(malToken), 100);

    assertTrue(config.isSupportedAsset(address(malToken)));

    vm.expectCall(address(malToken), abi.encodeWithSelector(
     MalToken.transferFrom.selector, address(0), address(0), 10
    ));

    IERC20(address(malToken)).transferFrom(address(0), address(0), 10);

  }
}

contract MalToken {
  function transferFrom(address from, address to, uint amount) external {
    // drain funds 
  }
}

But the Likelihood of this happening

Requires permission to call addNewSupportedAsset (MANAGER role holder)
If improper access controls, could be exploited

Tools Used

Manual review

Recommended Mitigation

In addNewSupportedAsset, validate asset addresses are proper ERC20 contracts before adding:

function addNewSupportedAsset(address asset, uint256 depositLimit) external {

+ require(ERC20(asset).totalSupply() > 0, "Invalid ERC20 token");

  // Additional ERC20 validity checks

  // ...

}

This prevents invalid addresses from being added.

Assessed type

Invalid Validation

c4-pre-sort commented 10 months ago

raymondfam marked the issue as insufficient quality report

c4-pre-sort commented 10 months ago

raymondfam marked the issue as duplicate of #69

c4-judge commented 9 months ago

fatherGoose1 changed the severity to QA (Quality Assurance)