Lack of protection when withdrawing Static Atoken

[code423n4]

Lines of code

https://github.com/reserve-protocol/protocol/blob/9ee60f142f9f5c1fe8bc50eef915cf33124a534f/contracts/plugins/assets/aave/StaticATokenLM.sol#L324-L365

Vulnerability details

Impact

The Aave plugin is associated with an ever-increasing exchange rate. The earlier a user wraps the AToken, the more Static Atoken will be minted and understandably no slippage protection is needed.

However, since the rate is not linearly increasing, withdrawing the Static Atoken (following RToken redemption) at the wrong time could mean a difference in terms of the amount of AToken redeemed. The rate could be in a transient mode of non-increasing or barely increasing and then a significant surge. Users with an equivalent amount of Static AToken making such calls at around the same time could end up having different percentage gains.

Although the user could always deposit and wrap the AToken again, it's not going to help if the wrapping were to encounter a sudden surge (bad timing again) thereby thwarting the intended purpose.

Proof of Concept

https://github.com/reserve-protocol/protocol/blob/9ee60f142f9f5c1fe8bc50eef915cf33124a534f/contracts/plugins/assets/aave/StaticATokenLM.sol#L338-L355

        uint256 userBalance = balanceOf(owner);

        uint256 amountToWithdraw;
        uint256 amountToBurn;

        uint256 currentRate = rate();
        if (staticAmount > 0) {
            amountToBurn = (staticAmount > userBalance) ? userBalance : staticAmount;
            amountToWithdraw = _staticToDynamicAmount(amountToBurn, currentRate);
        } else {
            uint256 dynamicUserBalance = _staticToDynamicAmount(userBalance, currentRate);
            amountToWithdraw = (dynamicAmount > dynamicUserBalance)
                ? dynamicUserBalance
                : dynamicAmount;
            amountToBurn = _dynamicToStaticAmount(amountToWithdraw, currentRate);
        }

        _burn(owner, amountToBurn);

As can be seen in the code block of function _withdraw above, choosing staticAmount > 0 will have a lesser amount of AToken to withdraw when the currenRate is stagnant.

https://github.com/reserve-protocol/protocol/blob/9ee60f142f9f5c1fe8bc50eef915cf33124a534f/contracts/plugins/assets/aave/StaticATokenLM.sol#L297-L299

    function _staticToDynamicAmount(uint256 amount, uint256 rate_) internal pure returns (uint256) {
        return amount.rayMul(rate_);
    }

Similarly, choosing dynamicAmount > 0 will have a higher than expected amount of Static Atoken to burn.

https://github.com/reserve-protocol/protocol/blob/9ee60f142f9f5c1fe8bc50eef915cf33124a534f/contracts/plugins/assets/aave/StaticATokenLM.sol#L293-L295

    function _dynamicToStaticAmount(uint256 amount, uint256 rate_) internal pure returns (uint256) {
        return amount.rayDiv(rate_);
    }

Tools Used

Manual

Recommended Mitigation Steps

Consider implementing slippage protection on StaticATokenLM._withdraw so that users could opt for the minimum amount of AToken to receive or the maximum amount of Static Atoken to burn.

Assessed type

Timing

[c4-judge]

thereksfour marked the issue as primary issue

[c4-sponsor]

tbrent marked the issue as sponsor acknowledged

[julianmrodri]

HI, as mentioned before we acknowledge the existence of this issue.

But on the other hand we will not implement fixes or changes. We believe it is the responsibility of the user to decide when to wrap/unwrap these tokens and these interactions are in general outside of the protocol behavior.

In addition to this the rate is accesible for the user to make that decision, and we don't expect these rates to increase abruptly for this wrapper, so in reality we might be adding a feature that will probably not be used in practice.

It is important to remark this is something that exists in any wrapper for rebasing tokens we use, wether it is our own, or developed by other protocol teams. And in generally we don't see implemented in those wrappers slippage protection or a feature like the one suggested here.

[c4-judge]

thereksfour marked the issue as satisfactory

[c4-judge]

thereksfour marked the issue as selected for report