search

code-423n4 / 2022-05-rubicon-findings

5 stars 2 forks source link

Share Withdrawal Re-Entrancy #410

Closed code423n4 closed 2 years ago

code423n4 commented 2 years ago

Lines of code

https://github.com/code-423n4/2022-05-rubicon/blob/main/contracts/rubiconPools/BathToken.sol#L595 https://github.com/code-423n4/2022-05-rubicon/blob/main/contracts/rubiconPools/BathToken.sol#L598

Vulnerability details

BTN-02M: Share Withdrawal Re-Entrancy

File Lines Type
BathToken.sol L595, L598 Checks-Effects-Interactions Pattern

Description

The _withdraw mechanism of the BathToken does not conform to the Checks-Effects-Interactions pattern as it distributes bonus token rewards prior to the redemption (i.e. burning) of shares. While this in and of itself is an issue in the case of re-entrant EIP-777 tokens, it is an even more valid vulnerability as the system is expected to handle native asset rewards and thus cause re-entrancies via native asset transfers as well like the VestingWallet implementation and corresponding release implementation.

Impact

The re-entrant call can lead to redemptions beyond the first to have unfair evaluations as the original shares will not have exited the system at that point in time.

Solution (Recommended Mitigation Steps)

We advise the code to conform to the CEI pattern by performing any external calls (such as reward distributions) after the shares have been burned and all state variables have been properly updated. As an additional security measure, we advise the non-reentrant modifier by OpenZeppelin's ReentrancyGuard contract to be applied throughout the codebase.

PoC

Issue is deducible by inspecting the relevant lines referenced in the issue, making note of the distributeBonusTokenRewards function execution prior to the _burn operation.

Tools

Manual inspection of the codebase.

bghughes commented 2 years ago

Good recommendation to use Reentrancy Gaurd and Duplicate of #283