[H-12] 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#L661-L665

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 getRedemptionFeeWithDecay function

    function getRedemptionFeeWithDecay(
        uint256 _stETHToRedeem
    ) external view override returns (uint256) {
        return _calcRedemptionFee(getRedemptionRateWithDecay(), _stETHToRedeem);
    }

Proof of Concept

Exploit of getRedemptionFeeWithDecay function in foundry

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

Log Results in foundry

forge test --match-test "test_Reen10" --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_Reen10() (gas: 42494)
Traces:
  [42494] CDPManagerGovernanceTest::test_Reen10() 
    ├─ [5429] Utilities::getNextUserAddress() 
    │   └─ ← 0x9aF2E2B7e57c1CD7C68C5C3796d8ea67e0018dB7
    ├─ [0] VM::prank(0x9aF2E2B7e57c1CD7C68C5C3796d8ea67e0018dB7) 
    │   └─ ← ()
    ├─ [16783] AccruableCdpManager::getRedemptionFeeWithDecay(1000000000000000000 [1e18]) [staticcall]
    │   └─ ← 5000000000000000 [5e15]
    ├─ [45] AccruableCdpManager::fallback{value: 1000000000000000000}() 
    │   └─ ← "EvmError: Revert"
    └─ ← ()

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

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