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

Missing check on `transfer` return value may result in stakers rewards being lost

spacelord47

Summary

When a staker try to claim rewards by calling LiquidationPool.claimRewards function, this function uses IERC20.transfer to transfer rewards, but the return value of this call is never checked.

Issue details

ERC20 implementations known for its inconsistency. transfer for some implementations can return false instead of revering.

Also the EIP-20 documentation states that:

Callers MUST handle false from returns (bool success). Callers MUST NOT assume that false is never returned!

But in LiquidationPool.claimRewards we can see that an "unsafe" transfer call performed at line 175:

164: function claimRewards() external {
165: ITokenManager.Token[] memory _tokens = ITokenManager(tokenManager).getAcceptedTokens();
166: for (uint256 i = 0; i < _tokens.length; i++) {
167: ITokenManager.Token memory _token = _tokens[i];
168: uint256 _rewardAmount = rewards[abi.encodePacked(msg.sender, _token.symbol)];
169: if (_rewardAmount > 0) {
170: delete rewards[abi.encodePacked(msg.sender, _token.symbol)];
171: if (_token.addr == address(0)) {
172: (bool _sent,) = payable(msg.sender).call{value: _rewardAmount}("");
173: require(_sent);
174: } else {
175: IERC20(_token.addr).transfer(msg.sender, _rewardAmount);
176: }
177: }
178:
179: }
180: }

Before calling transfer, the Reward record removed from rewards(line 170). So, if transfer failed and returned false, claimRewards will succeed, but the user will not receive expected rewards.

It can't be guaranteed that newly added ERC20 tokens will not use return false, so there is no reason to ignore this case.

NOTE: this issue also reported in Aderyn Analysis Report(L-3), which includes more instances of "unsafe transfer". But I want to underline this particular case (and mark it as Medium risk) because the impact is different, and not explained in the report.

Instances in LiquidationPoolManager:

  1. funds might be not transferred to Protocol

  2. The funds will still be held by the Protocol's owned LiquidationPoolManager

Instance in LiquidationPool.claimRewards:

  1. funds might be not transferred to users

  2. funds will stuck in LiquidationPool

  3. Reward record will be removed, there is no events (or any other "onchain" tracking). So, there is no reliable way to recover lost rewards.

Impact

Reward tokens for stakers can be lost.

Recommendation

SafeERC20 already integrated by the Protocol. So we can just replace usages of transfer with its "safe" version - safeTransfer:

diff --git a/contracts/LiquidationPool.sol b/contracts/LiquidationPool.sol
--- a/contracts/LiquidationPool.sol (revision c12272f2eec533019f2d255ab690f6892027f112)
+++ b/contracts/LiquidationPool.sol (date 1704278488810)
@@ -172,7 +172,7 @@
(bool _sent,) = payable(msg.sender).call{value: _rewardAmount}("");
require(_sent);
} else {
- IERC20(_token.addr).transfer(msg.sender, _rewardAmount);
+ IERC20(_token.addr).safeTransfer(msg.sender, _rewardAmount);
}
}
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.