[H-02] The Ocean contract and the onERC721Received function is vulnerable to read-only re-entrancy

[c4-bot-5]

Lines of code

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

Vulnerability details

Impact

The contract.function called Ocean.onERC721Received is vulnerable to read-only re-entrancy. The read-only re-entrancy is possible if the contract function is called externally from another contract.

What follows are the functions that are traversed through via calling the onERC721Received function in the Ocean contract.

Ocean.onERC721Received(address,address,uint256,bytes) (src/ocean/Ocean.sol#309-315):

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

- Ocean._ERC721InteractionStatus (src/ocean/Ocean.sol#109) was read at _ERC721InteractionStatus == INTERACTION (src/ocean/Ocean.sol#310)
This variable was written at (after external call):

- _ERC721InteractionStatus = NOT_INTERACTION (src/ocean/Ocean.sol#892)

- Ocean._ERC721InteractionStatus (src/ocean/Ocean.sol#109) was read at IERC721Receiver.onERC721Received.selector (src/ocean/Ocean.sol#311)
This variable was written at (after external call):

- _ERC721InteractionStatus = NOT_INTERACTION (src/ocean/Ocean.sol#892)

- Ocean._ERC721InteractionStatus (src/ocean/Ocean.sol#109) was read at 0 (src/ocean/Ocean.sol#313)
This variable was written at (after external call):

- _ERC721InteractionStatus = NOT_INTERACTION (src/ocean/Ocean.sol#892)

Proof of Concept

The onERC721Received function that is vulnerable to re-entrancy is on lines 309 to 315 of the Ocean contract and is depicted as follows.

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

The exploit written for this read-only re-entrancy of the onERC721Received function was written in an attacker contract.

This attacker contract can be found in a test file provided by shell-protocol as follows.

src/test/fork/TestCurve2PoolAdapter.t.sol

The function was written in foundry by me and is as follows.

function testCallB() 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.onERC721Received(user1,user2,uint256(1e18),bytes("0x1e18"));
        msg.sender.call {value: 1e18} ("0x1e18");
        return (statementA, balanceBefore, statementB, msg.sender.balance, statementC, msg.sender.balance-balanceBefore);
    }

The log results to prove the withdrawn amount of 1 ether or 1e18 in foundry is depicted below.

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

Running 1 test for src/test/fork/TestCurve2PoolAdapter.t.sol:TestCurve2PoolAdapter
[PASS] testCallB() (gas: 17340)
Traces:
  [17340] TestCurve2PoolAdapter::testCallB()
    ├─ [2720] Ocean::onERC721Received(0x0000000000000000000000000000000050f04948, 0x000000000000000000000000000000004a50b67b, 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.43s

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