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

Re-entrancy vulnerability in `LiquidationPool::decreasePosition`, user can obtain a large amount of TST and EUROs.

dopeflamingo

Summary

LiquidationPool::decreasePosition does not follow the CEI (Checks --> Effects --> Interactions) pattern and as a result, a bad actor can leverage this flaw in order to gain a large amount of TST and EUROs.

Vulnerability Details

https://github.com/Cyfrin/2023-12-the-standard/blob/91132936cb09ef9bf82f38ab1106346e2ad60f91/contracts/LiquidationPool.sol#L152-L160

The code shows how decreasing the position of your stake in the protocol is handled. As you can see, the mapping of the positions is only updated after the safeTransfer() has occurred.

require(_tstVal <= positions[msg.sender].TST && _eurosVal <= positions[msg.sender].EUROs, "invalid-decr-amount");
if (_tstVal > 0) {
IERC20(TST).safeTransfer(msg.sender, _tstVal);
positions[msg.sender].TST -= _tstVal;
}
if (_eurosVal > 0) {
IERC20(EUROs).safeTransfer(msg.sender, _eurosVal);
positions[msg.sender].EUROs -= _eurosVal;
}

Consider the following scenario:

Step 1: A user has 100 EUROs and 100 TST in their position, through the use of a contract. This sets positions[msg.sender].EUROs and positions[msg.sender].TST equal to 100.

Step 2: They call the decreasePosition function through a smart contract passing _tstValue and _eurosVal as 100 in order to empty their position.

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) {
IERC20(TST).safeTransfer(msg.sender, _tstVal);
positions[msg.sender].TST -= _tstVal;
}
if (_eurosVal > 0) {
IERC20(EUROs).safeTransfer(msg.sender, _eurosVal);
positions[msg.sender].EUROs -= _eurosVal;
}
if (empty(positions[msg.sender])) deletePosition(positions[msg.sender]);
}

Step 3: As the check require(_tstVal <= positions[msg.sender].TST && _eurosVal <= positions[msg.sender].EUROs, "invalid-decr-amount"); holds true, the function proceeds.

Step 4: The contract is then sent the _tstVal and _eurosVal through the safeTransfer() method. As this is an external call, the fallback of the attack contract is triggered. The funds have been sent and the state of positions[msg.sender] is not updated.

Step 5: Go back step 2. The check still passes as the state is out of date.

Step 6: This means the user can have a large amount of EUROs and TSTs gained.

Impact

The reentrancy vulnerability could allow an attacker to reenter the decreasePosition function before state changes are finalized. This could potentially enable an attacker to repeatedly call the function, manipulating the state and causing loss or unauthorized transfer of funds. The attack can gain a disproportionate amount of TST and EUROs unfairly.

Tools Used

Manual Review

Recommendations

Adjust the code so it follows CEI.

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) {
- IERC20(TST).safeTransfer(msg.sender, _tstVal);
- positions[msg.sender].TST -= _tstVal;
​
+ positions[msg.sender].TST -= _tstVal;
+ IERC20(TST).safeTransfer(msg.sender, _tstVal);
}
if (_eurosVal > 0) {
IERC20(EUROs).safeTransfer(msg.sender, _eurosVal);
positions[msg.sender].EUROs -= _eurosVal;
​
- IERC20(EUROs).safeTransfer(msg.sender, _eurosVal);
- positions[msg.sender].EUROs -= _eurosVal;
​
+ positions[msg.sender].EUROs -= _eurosVal;
+ IERC20(EUROs).safeTransfer(msg.sender, _eurosVal);
}
if (empty(positions[msg.sender])) deletePosition(positions[msg.sender]);
}

Or Alternatively, use a reentrancy guard nonReetrant modifier, see: https://github.com/OpenZeppelin/openzeppelin-contracts/blob/master/contracts/utils/ReentrancyGuard.sol

Updates

Lead Judging Commences

hrishibhat Lead Judge almost 2 years 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.