[H-08] Read-Only Reentrancy in the CdpManager contract

[c4-submissions]

Lines of code

https://github.com/code-423n4/2023-10-badger/blob/f2f2e2cf9965a1020661d179af46cb49e993cb7e/packages/contracts/contracts/CdpManager.sol#L567-L572

Vulnerability details

Impact

The read-only reentrancy is a reentrancy scenario where a view function is reentered, which in most cases is unguarded as it does not modify the contract’s state. However, if the state is inconsistent, wrong values could be reported. Other protocols relying on a return value can be tricked into reading the wrong state to perform unwanted actions.

Protocols integrate with other DeFi protocols to read token prices or read prices of wrapped tokens minted on particular protocols. This is possible when any lending protocol supports the pool tokens from other protocols as collateral or allows staking. All these integrations will require reading pool token prices or wrapped token prices.

Vulnerable getSystemDebt function

    /// @notice Get the sum of debt units assigned to all Cdps within eBTC system
    /// @dev It is actually the `systemDebt` value of the ActivePool.
    /// @return entireSystemDebt entire system debt accounting value
    function getSystemDebt() public view returns (uint256 entireSystemDebt) {
        return _getSystemDebt();
    }

Proof of Concept

Attackers misuse these situations to perform read-only reentrancy attacks using price manipulation. 1. An attacker contract will call a contract that can provide reentrancy. Other protocols must also be reading some values from it. So, let’s call it a reentrant contract. 2. The reentrant contract will fall back to the attacker contract, and the fallback method logic will be executed. 3. The attacker contract will now call a target DeFi protocol contract, as nothing prevents it from calling it. 4. Target protocol, as assumed earlier, will be reading some value from the compromised reentrant contract. 5. By the time Step 3 and Step 4 finish, the attacker would have already exploited the target contract and will simply proceed to let Step 1 and Step 2 finish.

Exploit of getSystemDebt function in foundry

 function test_Reen6() external {
        address me = _utils.getNextUserAddress();
        vm.prank(me);
        cdpManager.getSystemDebt();
        // fallback
        payable(address(cdpManager)).call {value: 1e18} ("");
    }

Log results in foundry

forge test --match-test "test_Reen6" --ffi -vvvv
[⠔] Compiling...
[⠰] Compiling 1 files with 0.8.17
[⠘] Solc 0.8.17 finished in 8.15s

Running 1 test for foundry_test/CDPManager.governance.t.sol:CDPManagerGovernanceTest
[PASS] test_Reen6() (gas: 31332)
Traces:
  [31332] CDPManagerGovernanceTest::test_Reen6() 
    ├─ [5429] Utilities::getNextUserAddress() 
    │   └─ ← 0x9aF2E2B7e57c1CD7C68C5C3796d8ea67e0018dB7
    ├─ [0] VM::prank(0x9aF2E2B7e57c1CD7C68C5C3796d8ea67e0018dB7) 
    │   └─ ← ()
    ├─ [5650] AccruableCdpManager::getSystemDebt() [staticcall]
    │   ├─ [2402] ActivePool::getSystemDebt() [staticcall]
    │   │   └─ ← 0
    │   └─ ← 0
    ├─ [45] AccruableCdpManager::fallback{value: 1000000000000000000}() 
    │   └─ ← "EvmError: Revert"
    └─ ← ()

Test result: ok. 1 passed; 0 failed; finished in 7.67ms

Tools Used

VS Code. Foundry.

Recommended Mitigation Steps

Mitigation methods vary and depend on whether a new contract is being written or whether a contract that’s already deployed is being read.

For new contracts, a read-only reentrancy guard needs to be added. This works by verifying whether the reentrancy guard has not been locked. If it has, then an error is thrown (for example, Euler Finance).

The lock can be made public so those contracts which are calling the contract can check if the contract is already in lock mode.

For the rest, a function with a reentrant modifier can be called first. If it fails, the contract will be in locked mode, and reading from it should be avoided.

Assessed type

Reentrancy

[c4-pre-sort]

bytes032 marked the issue as insufficient quality report

[c4-judge]

jhsagd76 marked the issue as unsatisfactory: Invalid