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

No Reward Available if `ERC20.transfer` Reverts or Returns False in `LiquidationPool`, Leading to Loss of Rewards for Stakers and Loss of Funds for Protocol

n0kto

Description

If a token transfer operation reverts or returns false, the associated rewards are not distributed. This issue could lead to a loss of rewards for stakers. Additionally, certain upgradeable ERC20 tokens might change their behavior, impacting the reward distribution.

Real Example: The ZRXToken serves as an example where a token might return false on transfer.

Impact

Loss of rewards for stakers. While the likelihood of accepted tokens being administratively managed by the protocol is low, the impact is notable.

Proof of Concept

Foundry PoC
function testTokenReturningFalseForRewardsClaiming() public {
// Stakers put EUROs and TST in staking
vm.startPrank(staker);
EUROs.approve(address(pool), 10000e18);
TST.approve(address(pool), 10000e18);
pool.increasePosition(9000e18, 9000e18);
vm.stopPrank();
​
TokenTransferReturningFalseMock fakeToken = new TokenTransferReturningFalseMock(
"Fake Token",
"FTK",
18
);
ChainlinkMock CL_FakeToken_USD = new ChainlinkMock("FTK / USD");
CL_FakeToken_USD.setPrice(USD_PRICE);
​
vm.prank(protocol);
tokenManager.addAcceptedToken(
address(fakeToken),
address(CL_FakeToken_USD)
);
​
// Use fakeToken in vault (like vault's user sending fakeToken to the vault)
fakeToken.mint(address(vault), 10000e18);
​
// Mint EUROs
vm.prank(vaultUser);
vault.mint(vaultUser, 7000e18);
​
// Price decrease, and the vault is liquidated
vm.startPrank(protocol);
CL_FakeToken_USD.setPrice(1e17);
assertTrue(vault.undercollateralised());
​
liquidationPoolManager.runLiquidation(vaultID);
vm.stopPrank();
​
// Staker claims their reward
vm.prank(staker);
pool.claimRewards();
​
// Staker has no rewards, and the "garbage collector" sends all the money to the protocol
assertEq(fakeToken.balanceOf(protocol), 10000e18);
assertEq(fakeToken.balanceOf(staker), 0);
}

Recommended Mitigation

Check the return value of the transfer operation and delete the rewards afterward. As this breaks the CEI (Check-Effect-Interaction) concept, add a ReentrancyGuard. A suggested improvement would be to implement a deadline for reward collection, allowing the protocol to retrieve funds if users don't collect rewards within a specified timeframe. This would prevent tokens from being stuck in case of a bug in an ERC20 contract.

-contract LiquidationPool is ILiquidationPool {
+contract LiquidationPool is ILiquidationPool, ReentrancyGuard {
.
.
.
- function claimRewards() external {
+ function claimRewards() external nonReentrant {
ITokenManager.Token[] memory _tokens = ITokenManager(tokenManager)
.getAcceptedTokens();
for (uint256 i = 0; i < _tokens.length; i++) {
ITokenManager.Token memory _token = _tokens[i];
// e check le mapping de reward par token.symbol
uint256 _rewardAmount = rewards[
abi.encodePacked(msg.sender, _token.symbol)
];
if (_rewardAmount > 0) {
- delete rewards[abi.encodePacked(msg.sender, _token.symbol)];
+ bool sent;
if (_token.addr == address(0)) {
- (bool _sent, ) = payable(msg.sender).call{
+ (bool sent, ) = payable(msg.sender).call{
value: _rewardAmount
}("");
- require(_sent);
} else {
- IERC20(_token.addr).transfer(msg.sender, _rewardAmount);
+ sent = IERC20(_token.addr).transfer(msg.sender, _rewardAmount);
}
​
+ if (sent){
+ delete rewards[abi.encodePacked(msg.sender, _token.symbol)];
+ }
}
}
}
Updates

Lead Judging Commences

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

unchecked-transfer

informational/invalid

Support

FAQs

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