The Standard
The Standard
DeFiHardhat
20,000 USDC
View results
Previous Next
Submission Details
Severity: low
Invalid

there's a risk of reentrancy attacks due to the order of operations and LiquidationPool::decreasePosition()distributeFees function in LiquidationPool::increasePosition()

0xluckyluke

Summary

The issue is in the LiquidationPool.increasePosition(), LiquidationPool::decreasePosition(), SmartVaultManagerV5::mint() function, where there's a risk of reentrancy attacks due to the order of operations in the increasePosition function.

Vulnerability Details

The LiquidationPool::increasePosition(), LiquidationPool::decreasePosition, and SmartVaultManagerV5::mint() functions update state variables after interacting with external contracts, making them vulnerable to reentrancy attacks. An attacker could exploit this by repeatedly calling the function before state variables are updated.

Impact

This vulnerability may allow unexpected behavior, as an attacker could manipulate the contract's state during the execution of increasePosition and decreasePosition.

Tools Used

Slither

Recommendations

  1. Reorder Operations:
    Change the order of operations in LiquidationPool::distributeFees to update state variables before making external calls, reducing the risk of reentrancy attacks.

function increasePosition(uint256 _tstVal, uint256 _eurosVal) external {
require(_tstVal > 0 || _eurosVal > 0);
consolidatePendingStakes();
​
ILiquidationPoolManager(manager).distributeFees();
- if (_tstVal > 0) IERC20(TST).safeTransferFrom(msg.sender, address(this), _tstVal);
- if (_eurosVal > 0) IERC20(EUROs).safeTransferFrom(msg.sender, address(this), _eurosVal);
pendingStakes.push(PendingStake(msg.sender, block.timestamp, _tstVal, _eurosVal));
addUniqueHolder(msg.sender);
+ if (_tstVal > 0) IERC20(TST).safeTransferFrom(msg.sender, address(this), _tstVal);
+ if (_eurosVal > 0) IERC20(EUROs).safeTransferFrom(msg.sender, address(this), _eurosVal);
​
}
function decreasePosition(uint256 _tstVal, uint256 _eurosVal) external {
consolidatePendingStakes();
ILiquidationPoolManager(manager).distributeFees();
require(_tstVal <= positions[msg.sender].TST && _eurosVal <= positions[msg.sender].EUROs, "invalid-decr-amount");
if (_tstVal > 0) {
+ positions[msg.sender].TST -= _tstVal;
IERC20(TST).safeTransfer(msg.sender, _tstVal);
- positions[msg.sender].TST -= _tstVal;
}
if (_eurosVal > 0) {
+ positions[msg.sender].EUROs -= _eurosVal;
IERC20(EUROs).safeTransfer(msg.sender, _eurosVal);
- positions[msg.sender].EUROs -= _eurosVal;
}
if (empty(positions[msg.sender])) deletePosition(positions[msg.sender]);
}
function mint() external returns (address vault, uint256 tokenId) {
tokenId = lastToken + 1;
- _safeMint(msg.sender, tokenId);
lastToken = tokenId;
vault = ISmartVaultDeployer(smartVaultDeployer).deploy(address(this), msg.sender, euros);
smartVaultIndex.addVaultAddress(tokenId, payable(vault));
IEUROs(euros).grantRole(IEUROs(euros).MINTER_ROLE(), vault);
IEUROs(euros).grantRole(IEUROs(euros).BURNER_ROLE(), vault);
emit VaultDeployed(vault, msg.sender, euros, tokenId);
+ _safeMint(msg.sender, tokenId);
}

  1. Use Checks-Effects-Interactions Pattern:
    Follow a pattern where checks and state variable updates come before interactions with external contracts to minimize reentrancy risks.

// Example of Checks-Effects-Interactions pattern
if (tstTotal > 0) {
// Checks
for (uint256 i = 0; i < holders.length; i++) {
// Effects
address _holder = holders[i];
positions[_holder].EUROs += _amount * positions[_holder].TST / tstTotal;
}
// Interactions
IERC20(EUROs).safeTransferFrom(msg.sender, address(this), _amount);
}

By making these changes, the contract can be more secure against reentrancy attacks.

Updates

Lead Judging Commences

hrishibhat Lead Judge over 1 year ago
Submission Judgement Published
Invalidated
Reason: Too generic
Assigned finding tags:

informational/invalid

Support

FAQs

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