Lines of code

https://github.com/code-423n4/2022-08-olympus/blob/main/src/libraries/TransferHelper.sol#L10-L33

Vulnerability details

Impact

TransferHelper.sol and solmate won't check if the token is a contract or not.

A hacker could set traps for non existing tokens to steal future funds from users.

Proof of Concept

The safeTransfer() functions used in the contract are wrappers around the solmate library. Solmate will not check for contract existance.

File: src/libraries/TransferHelper.sol
function safeTransferFrom(
    ERC20 token,
    address from,
    address to,
    uint256 amount
) internal {
    (bool success, bytes memory data) = address(token).call(
        abi.encodeWithSelector(ERC20.transferFrom.selector, from, to, amount)
    );

    require(success && (data.length == 0 || abi.decode(data, (bool))), "TRANSFER_FROM_FAILED");
}

function safeTransfer(
    ERC20 token,
    address to,
    uint256 amount
) internal {
    (bool success, bytes memory data) = address(token).call(
        abi.encodeWithSelector(ERC20.transfer.selector, to, amount)
    );

    require(success && (data.length == 0 || abi.decode(data, (bool))), "TRANSFER_FAILED");
}

Tools Used

Manual review.

Recommended Mitigation Steps

Looking at OpenZeppelin's implementation, a check is made on the code for the token address.

https://github.com/OpenZeppelin/openzeppelin-contracts/blob/master/contracts/utils/Address.sol#L36-L42

address(token).code.length > 0

Consider using OpenZeppelin or checking for contract existance manually inside TransferHelper.sol.

code-423n4 / 2022-08-olympus-findings