M-01 redeem Lacks Slippage Protection

[ColePBryan]

The redeem function in Escrow.sol contains the logic to determine the amount of GRT rewards an indexer will receive as the minimum of the currently available escrow balance and the aggregated amount in the voucher. This allows slippage to occur against the indexer (i.e., the indexer may get less than what is deemed acceptable).

The redeem function does not allow the indexer to specify an expected amount of rewards to receive. Therefore, it is possible that the indexer receives fewer rewards than expected, at which point the voucher that was worth more has been used and cannot be re-used. This could surprise the indexer, who may have otherwise expected the call to revert if the full reward amount could not be given to them.

Consider adding a parameter to the redeem function that allows the caller to specify an expected reward amount. If the calculated amount is not greater than or equal to the expected amount, the call should revert.

[ColePBryan]

Does adding slippage protection open up a somewhat niche but still real front-run attack. Specifically when a RAV is submitted for an EscrowAccount with low funds.
It would look something like this:

1. Escrow account has some arbitrarily low amount (lets say 0)
2. Indexer that is relying on slippage protection blindly submits RAV
3. Malicious actor takes all info from mempool and front runs the function call, but replaces the slippage value with 0 (or permitSlippage == true if we use a bool instead)
4. Because the new slippage value is zero the transaction goes through and the allocation ID is claimed for that sender
5. Indexer is now unable to claim their rewards

[ColePBryan]

Discussion from slack about solutions and trade-offs:

Me:

So right now possible solutions would be:

1. Revert if account is below redeemed amount
2. Allow underfunded redeem and leave it to the indexer to only call redeem if they are ok with the amount available in the account
3. Allow underfunded redeem and include a min_redeem amount with a singed proof

Pablo V.:

I think the slippage without protection (#​2), even with off-chain checking, could open unexpected edge cases, e.g. a redeem is called while some amount is thawing, and the transaction gets delayed until the thaw is completed... Whereas the revert (#​1) produces more predictable behavior but less flexibility, and adding the signed parameter (#​3) gives us the "best of both" worlds at the cost of uglier code and potential dead code in a future upgrade. So I don't see a clear winner, my instinct just pulls me towards #​1 as it's the simplest, or #​3 as it's the most "complete", but I think it's fine if you make the call for #​2.

Also for reference here is a previous discussion on this topic #5

[ColePBryan]

One more possible solution that was considered:

Split the redeem API into two functions:

• redeem() which would implement #​1
• redeemWithSlippage() which would implement #​3

This would allow indexer code to either choose either slippage protection with a tradeoff of extra gas or to choose all or nothing redeem (reverts if there is not enough to completely redeem RAV).

[ColePBryan]

Final decision is to leave the code as is (option #​2). This allows flexibility while maintaining a low gas cost. The documentation will be updated to reflect the expectation of the indexer.

Some context for the decision:

• Whenever a thaw commences and it threatens to reduce the escrow account below the owed amount, the indexer software should promptly act to close any pending RAVs with that gateway. Otherwise, the indexer should act as if anything being thawed has already been withdrawn.
• Complete transparency exists regarding an account: indexers can verify values anytime, and numerous events (like deposit, thaw, withdraw, redeem, and other account interactions) are signaled.
• The only methods to withdraw money from an account are either under the indexer's control (redeem()) or are subject to a time-lock (withdraw()).

[tkornuta-semiotic]

Pasting below a comment from OpenZeppelin reviewer from Slack, Aug/22/2023, 6:20AM PST:

Thanks for the great discussion @ColePBryan and Pablo and laying out the ideas clearly. I see a few considerations. Allow me to summarise to form an opinion.

On (#1): The purpose of setting a slippage is to protect an honest indexer from a rogue sender/signer pair. For instance, a rogue sender pairs with a rogue signer could sign away more value than they plan to pay to honest indexers. The rogue sender could either (a) deposit nothing/little for the indexer; or (b) pre-thaw and revoke the signer to invalidate a voucher. In both cases, the indexer gets nothing/little when redeeming. In (a), it can’t redeem again and the voucher is considered used; In (b), the voucher can’t be used unless the same signer is again authorised by the same/different sender who could deposit into the escrow account for the indexer. This may create further problems/confusion when different gateways re-issue (or issue more than once) RAVs against one allocationID. In (a), by allowing the indexer to drain however much is available is a better protection than reverting. In (b), there isn’t much the indexer can do at this stage. This argument would favour (#2) and (#3) over (#1). However the outcome of (#2) and (#3) may be the same.
On (#2): Due to the thawing=>withdraw mechanism, it seems that one can check off-chain the escrow balance and send a redeem transaction based on that information. However it is potentially possible this balance may change in that period of time. For instance, a rogue sender could start thawing the balance earlier and leave it there until there is a redeem transaction from the indexer account in the mempool. Then the sender could put a withdraw transaction right before the redeem. Thus the sender can trick indexer to redeem for nothing, in theory if timed well. This argument then favours (#3) over (#2). At least for (#3), there is a chance to retry.
On (#3): One argument against (#3) is that of the separation of concerns, i.e. the min_amount needs to interact with Escrow.sol and this variable is not useful to keep in AllocationIDTracker.sol. Indeed this interaction must happen to validate if the transaction needs to be reverted. That could involve an external call from the allocationIDTracker to the Escrow contract. But it can be done elegantly like what’s already there in verifyProof using msg.sender to represent the Escrow contract. It seems one does not really need to keep min_amount in the storage but merely use it in verifyProof, unless it could be useful later (idk, for refund? just thinking out loud).
So the preference here is (#3) despite that the outcome to the indexer may be the same under the unfortunate scenario of a rogue sender. By whitelisting senders, the protocol has already reduced the chance of rogue senders.