[M-15] Reentrancy in the BranchPort contract

[c4-submissions]

Lines of code

https://github.com/code-423n4/2023-09-maia/blob/f5ba4de628836b2a29f9b5fff59499690008c463/src/BranchPort.sol#L167-L185 https://github.com/code-423n4/2023-09-maia/blob/f5ba4de628836b2a29f9b5fff59499690008c463/src/BranchPort.sol#L188-L219

Vulnerability details

Impact

In a Re-entrancy attack, a malicious contract calls back into the calling contract before the first invocation of the function is finished. This may cause the different invocations of the function to interact in undesirable ways, especially in cases where the function is updating state variables after the external calls. This may lead to loss of funds, improper value updates, token loss, etc.

Proof of Concept

The replenishReserves function is vulnerable to reentrancy

Line 167-185
    function replenishReserves(address _token, uint256 _amount) external override lock {
        // Update Port Strategy Token Debt. Will underflow if not enough debt to repay.
        getPortStrategyTokenDebt[msg.sender][_token] -= _amount;

        // Update Strategy Token Global Debt. Will underflow if not enough debt to repay.
        getStrategyTokenDebt[_token] -= _amount;

        // Get current balance of _token
        uint256 currBalance = ERC20(_token).balanceOf(address(this));

        // Withdraw tokens from startegy
        IPortStrategy(msg.sender).withdraw(address(this), _token, _amount);

        // Check if _token balance has increased by _amount
        require(ERC20(_token).balanceOf(address(this)) - currBalance == _amount, "Port Strategy Withdraw Failed");

        // Emit DebtRepaid event
        emit DebtRepaid(msg.sender, _token, _amount);
    }

The replenishReserves function is vulnerable to reentrancy

// Line 188-219
    function replenishReserves(address _strategy, address _token) external override lock {
        // Cache Strategy Token Global Debt
        uint256 strategyTokenDebt = getStrategyTokenDebt[_token];

        // Get current balance of _token
        uint256 currBalance = ERC20(_token).balanceOf(address(this));

        // Get reserves lacking
        uint256 reservesLacking = _reservesLacking(strategyTokenDebt, _token, currBalance);

        // Cache Port Strategy Token Debt
        uint256 portStrategyTokenDebt = getPortStrategyTokenDebt[_strategy][_token];

        // Calculate amount to withdraw. The lesser of reserves lacking or Strategy Token Global Debt.
        uint256 amountToWithdraw = portStrategyTokenDebt < reservesLacking ? portStrategyTokenDebt : reservesLacking;

        // Update Port Strategy Token Debt
        getPortStrategyTokenDebt[_strategy][_token] = portStrategyTokenDebt - amountToWithdraw;
        // Update Strategy Token Global Debt
        getStrategyTokenDebt[_token] = strategyTokenDebt - amountToWithdraw;

        // Withdraw tokens from startegy
        IPortStrategy(_strategy).withdraw(address(this), _token, amountToWithdraw);

        // Check if _token balance has increased by _amount
        require(
            ERC20(_token).balanceOf(address(this)) - currBalance == amountToWithdraw, "Port Strategy Withdraw Failed"
        );

        // Emit DebtRepaid event
        emit DebtRepaid(_strategy, _token, amountToWithdraw);
    }

Tools Used

VS Code.

Recommended Mitigation Steps

It is recommended to add a Re-entrancy Guard to the functions making external calls. The functions should use a Checks-Effects-Interactions pattern. The external calls should be executed at the end of the function and all the state-changing must happen before the call.

Assessed type

Reentrancy

[c4-pre-sort]

0xA5DF marked the issue as duplicate of #879

[c4-pre-sort]

0xA5DF marked the issue as sufficient quality report

[c4-pre-sort]

0xA5DF marked the issue as low quality report

[c4-judge]

alcueca marked the issue as unsatisfactory: Out of scope

[alcueca]

No proof of impact, overinflated severity, bot race already alerting of reentrancy possibilities.