rvierdiiev - ERC5095.deposit doesn't check if received shares is less then provided amount
Opened this issue · 7 comments
rvierdiiev
medium
ERC5095.deposit doesn't check if received shares is less then provided amount
Summary
ERC5095.deposit doesn't check if received shares is less then provided amount. In some cases this leads to lost of funds.
Vulnerability Detail
The main thing with principal tokens is to buy them when the price is lower (you can buy 101 token while paying only 100 base tokens) as underlying price and then at maturity time to get interest(for example in one month you will get 1 base token in our case).
ERC5095.deposit function takes amount of base token that user wants to deposit and returns amount of shares that he received. To not have loses, the amount of shares should be at least bigger than amount of base tokens provided by user.
function deposit(address r, uint256 a) external override returns (uint256) {
if (block.timestamp > maturity) {
revert Exception(
21,
block.timestamp,
maturity,
address(0),
address(0)
);
}
uint128 shares = Cast.u128(previewDeposit(a));
Safe.transferFrom(IERC20(underlying), msg.sender, address(this), a);
// consider the hardcoded slippage limit, 4626 compliance requires no minimum param.
uint128 returned = IMarketPlace(marketplace).sellUnderlying(
underlying,
maturity,
Cast.u128(a),
shares - (shares / 100)
);
_transfer(address(this), r, returned);
return returned;
}While calling market place, you can see that slippage of 1 percent is provided.
uint128 returned = IMarketPlace(marketplace).sellUnderlying(
underlying,
maturity,
Cast.u128(a),
shares - (shares / 100)
);But this is not enough in some cases.
For example we have ERC5095 token with short maturity which provides 0.5% of interests.
userA calls deposit function with 1000 as base amount. He wants to get back 1005 share tokens. And after maturity time earn 5 tokens on this trade.
But because of slippage set to 1%, it's possible that the price will change and user will receive 995 share tokens instead of 1005, which means that user has lost 5 base tokens.
I propose to add one more mechanism except of slippage. We need to check if returned shares amount is bigger then provided assets amount.
Impact
Lost of funds.
Code Snippet
Provided above.
Tool used
Manual Review
Recommendation
Add this check at the end
require(returned > a, "received less than provided")
@sourabhmarathe which severity would you give this? And why?
Low severity on the basis that ultimately, user funds are not at risk in this case. However, it is still worth noting that this issue should be addressed using the recommended changes provided in this report.
Will keep it medium severity as the protocol agrees to fix the issues which can lead to a loss of funds according to the description.
Ehhhh its not quite loss of funds in the way described, but im on the fence with this one.
It kind of seems that this is a discussion about slippage given the "loss" is the same potential marginal loss as the other tickets that discuss slippage within the EIP-5095 interface.
That said, hes correct in saying that crossing the 1:1 ratio would imply a loss which might be good to avoid... And the proposed remediation isnt extremely gas heavy.
Buuut in some other extreme cases there also could be negative APYs? (Look at Europe yo)
E.g. if we did actually implement this, it would mean that we couldnt mechanically operate in a negative yield environment (though i think a lot of stuff would break as well anyways).
If accepted, I'd personally drop it to a low, given the "potential loss" is still a static amount related to the slippage params discussed in other tickets? 🤷
Escalate for 1 USDC
Reminder @Evert0x
You've created a valid escalation for 1 USDC!
To remove the escalation from consideration: Delete your comment.
To change the amount you've staked on this escalation: Edit your comment (do not create a new comment).
You may delete or edit your escalation comment anytime before the 48-hour escalation window closes. After that, the escalation becomes final.