BriVault
First Flight #52
Beginner FriendlySolidity
100 EXP
View results
Previous Next
Submission Details
Impact: low
Likelihood: low
Invalid

Reentrancy Risk in deposit function

kanyoro

Root + Impact

The deposit function violates the Checks-Effects-Interactions (CEI) security pattern. While this specific instance is not exploitable due to the code's structure, it represents a deviation from best practices and could lead to vulnerabilities during future code maintenance.

Description

  • Normal Behavior: A user calls deposit, the contract calculates shares, pulls the user's tokens, and mints the shares to the user.

  • The Problem: The deposit function executes two external safeTransferFrom calls (Interactions) before it updates the vault's internal state by calling _mint (Effect). This is a clear violation of the CEI pattern and at first glance appears to open a reentrancy attack vector.

// Root cause in src/briVault.sol
// The participantShares is calculated first
uint256 participantShares = _convertToShares(stakeAsset);
​
// Then, Interactions happen
@> IERC20(asset()).safeTransferFrom(msg.sender, participationFeeAddress, fee);
@> IERC20(asset()).safeTransferFrom(msg.sender, address(this), stakeAsset);
​
// Finally, the Effect happens last
@> _mint(msg.sender, participantShares);

Risk

Likelihood: Low

An exploit is not possible.

  • Reason 1: The critical value, participantShares, is calculated and stored in a local variable before the external calls.

  • Reason 2: A re-entrant call while possible, cannot modify the original function's local variables. The original call will resume and mint the correct, pre-calculated number of shares, preventing state corruption.

Impact: Low

  • Impact 1: A successful reentrancy attack could manipulate the share calculation, allowing the attacker to mint an excessive number of shares

  • Impact 2: Breaking of the vault's accounting and could lead to a direct loss of funds for other users.

Proof of Concept

The following demonstrates that re-entrancy is possible but has no negative effect. It uses a single test contract that also acts as the malicious ERC20 token. The test triggers a re-entrant deposit call but asserts that the final state totalSupply and balanceOf(vault) is correct, proving the state was not corrupted.

A test contract is set up that also inherits from MockERC20, allowing the test itself to act as the malicious asset token.
​
The transferFrom function is overridden within this test contract to serve as the re-entrancy hook.
​
This hook is programmed to detect when the briVault contract calls it to pull the stakeAsset.
​
When triggered, the hook immediately calls briVault.deposit() a second time before the first call has finished.
​
After the re-entrant call and the original call complete, the test asserts that the vault's final totalSupply and balanceOf are both correct (e.g., 19.7 ether of assets and 19.7 ether of shares).
​
This successful assertion proves that while re-entrancy is possible, the contract's state is not corrupted, and the vulnerability is possible.

Recommended Mitigation

The function should be refactored to strictly follow the Checks-Effects-Interactions pattern. This makes the code safer, easier to read, and aligns with security best practices, even though no exploit exists here.

// src/briVault.sol
function deposit(uint256 assets, address receiver) public override returns (uint256) {
// ... (Checks and calculations)
uint256 fee = _getParticipationFee(assets);
if (minimumAmount + fee > assets) {
revert lowFeeAndAmount();
}
uint256 stakeAsset = assets - fee;
stakedAsset[receiver] = stakeAsset;
uint256 participantShares = _convertToShares(stakeAsset);
​
- IERC20(asset()).safeTransferFrom(msg.sender, participationFeeAddress, fee);
- IERC20(asset()).safeTransferFrom(msg.sender, address(this), stakeAsset);
- _mint(msg.sender, participantShares);
​
+ // 1. EFFECT (Mint shares first)
+ _mint(msg.sender, participantShares);
+
+ // 2. INTERACTIONS (Transfer tokens last)
+ IERC20(asset()).safeTransferFrom(msg.sender, participationFeeAddress, fee);
+ IERC20(asset()).safeTransferFrom(msg.sender, address(this), stakeAsset);
​
emit deposited (receiver, stakeAsset);
return participantShares;
}
Updates

Appeal created

bube Lead Judge 11 months ago
Submission Judgement Published
Invalidated
Reason: Non-acceptable severity

Support

FAQs

Can't find an answer? Chat with us on Discord, Twitter or Linkedin.

Give us feedback!