code-423n4 / 2023-11-shellprotocol-findings

7 stars 7 forks source link

THE `OceanAdapter` CONTRACT CAN RECEIVE THE ERC1155 TOKENS BUT CAN NOT BE WITHDRAWN SINCE THERE IS NO APPROVAL FOR TOKEN TRANSFER THUS LOCKING THE TOKENS IN THE `Curve2PoolAdapter` CHILD CONTRACT #273

Closed c4-bot-3 closed 9 months ago

c4-bot-3 commented 9 months ago

Lines of code

https://github.com/code-423n4/2023-11-shellprotocol/blob/main/src/adapters/OceanAdapter.sol#L117-L119 https://github.com/code-423n4/2023-11-shellprotocol/blob/main/src/adapters/Curve2PoolAdapter.sol#L189-L192

Vulnerability details

Impact

The OceanAdapter is an abstract contract which is inherited by the Curve2PoolAdapter contract and the CurveTricrypticAdapter contract. The OceanAdapter contract has the onERC1155Received function implemented as shown below:

function onERC1155Received(address, address, uint256, uint256, bytes memory) public pure returns (bytes4) {
    return this.onERC1155Received.selector; //@audit-info - returns the selector of the onERC1155Received function to indicate the contract can accept erc1155
}

This indicates that the OceanAdapter contract or its children contracts can receive the ERC1155 tokens. But the issue here is that there is no logic implementation to withdraw the ERC1155 tokens from the OceanAdapter contract since the Curve2PoolAdapter has functionality to approve (approve ocean and primitive contracts) only the ERC20 tokens to be withdrawn and not the ERC1155 tokens as shown below:

function _approveToken(address tokenAddress) private {
    IERC20Metadata(tokenAddress).approve(ocean, type(uint256).max); //@audit-issue - return bool value is not checked
    IERC20Metadata(tokenAddress).approve(primitive, type(uint256).max);
} 

As a result the ERC1155 tokens transferred to the Curve2PoolAdapter (child of the OceanAdapter) will get stuck in the Curve2PoolAdapter since there is no approval to transfer them to either the ocean contract or the primitive contract. This could be loss of funds to the sender of the ERC1155 tokens to the Curve2PoolAdapter contract.

Proof of Concept

    function onERC1155Received(address, address, uint256, uint256, bytes memory) public pure returns (bytes4) {
        return this.onERC1155Received.selector;
    }

https://github.com/code-423n4/2023-11-shellprotocol/blob/main/src/adapters/OceanAdapter.sol#L117-L119

    function _approveToken(address tokenAddress) private {
        IERC20Metadata(tokenAddress).approve(ocean, type(uint256).max);
        IERC20Metadata(tokenAddress).approve(primitive, type(uint256).max);
    }

https://github.com/code-423n4/2023-11-shellprotocol/blob/main/src/adapters/Curve2PoolAdapter.sol#L189-L192

Tools Used

Manual Review and VSCode

Recommended Mitigation Steps

Hence it is recommended to either to remove the OceanAdapter.onERC1155Received if the adapter contracts are not expected to receive the ERC1155 tokens or if the adapter contracts need to receive the ERC1155 tokens then it is required to implement the logic to approve the ERC1155 tokens in the Curve2PoolAdapter to be transferred to the relevant recipient. And the relevant logic to transfer the ERC1155 tokens from the Curve2PoolAdapter should also be implemented in the respective contracts.

Assessed type

Other

c4-pre-sort commented 9 months ago

raymondfam marked the issue as insufficient quality report

c4-pre-sort commented 9 months ago

raymondfam marked the issue as duplicate of #20

c4-judge commented 9 months ago

0xA5DF changed the severity to QA (Quality Assurance)

c4-judge commented 9 months ago

0xA5DF marked the issue as grade-c

0xA5DF commented 9 months ago

Moved the points to #277

c4-judge commented 8 months ago

0xA5DF marked the issue as grade-b