Backd contest - sseefried's results

Lines of code

https://github.com/code-423n4/2022-04-backd/blob/c856714a50437cb33240a5964b63687c9876275b/backd/contracts/actions/topup/TopUpAction.sol#L727-L729

Vulnerability details

Impact

A Staker -- that has their top-up position removed after execute is called by a Keeper -- can always cause the transaction to revert. They can do this by deploying a smart contract to the payer address that has implemented a receive() function that calls revert(). The revert will be triggered by the following lines in execute

if (vars.removePosition) {
    gasBank.withdrawUnused(payer);
}

This will consume some gas from the keeper while preventing them accruing any rewards for performing the top-up action.

Proof of Concept

I have implemented a PoC in a fork of the contest repo. The attacker's contract can be found here.

Tools Used

Manual inspection

Recommend Mitigation Steps

To prevent this denial of service attack some way of blacklisting badly behaved Stakers should be added.

QA Report

Remarks/Recommendations

• The test suite was a joy to work with
• functions and data types could have had more documentation in places

Low Risk: No removeStakerVault function provided

At some point in the future a particular token may be found to be vulnerable to some kind of attack, or not behave in a reasonable manner. It should be possible to remove a Staker Vault in these circumstances. At present there is no way to remove a staker vault, only add them.

Low Risk: Reentrancy possible in execute but appears non-exploitable

Reentrancy is possible on line 727 of function execute because ETH is transfered. An attacker can point the beneficiary parameter to a smart contract with a receive() function implemented which can call back into backd contracts.

It does not seem that this reentrancy can be exploited since no state changes occur after the transfer of ETH. However, I have limited experience as an auditor and thought it would be best to point it out.

Low Risk: Don't use safeApprove in TopUpLibrary.sol

There are multiple uses of safeApprove in TopAction.sol. It has similar issues to approve where careful transaction ordering by an attacker could lead to uses of the old and the new allowance.

OpenZeppelin have deprecated this function here. A deeper discussion of the issue can be found in this GitHub Issue.

Low Risk: Functions which use Preparable.execute can be called by anyone

A number of functions in the code base indirectly call Preparable.execute but can be executed by anyone. I'll refer to this family of functions as execute*. This appears pretty safe since calls to prepare and reset are all permissioned.

I have not found an attack vector but I worry that it might be possible for an attacker to selectively order several contract calls, interspersing them with calls to execute* to gain some kind of advantage. The deadlines set by prepare are publicly viewable so the following example is feasible:

Example:

• wait until deadline has passed
• perform contract call that uses old state
• call execute*
• perform contract call that uses new state, presumably to some advantage

The way to mitigate this problem would be to make execute* functions permissioned. The permission could be less restricive that requiring GOVERNANCE or CONTROLLER permissions, but more restrictive than allowing anyone on the Ethereum network.

Non critical: AddressProvider.addStakerVault can only return true or revert

Function AddressProvider.addStakerVault can only return true or revert. Why return a boolean value at all if false can never be returned?

Non-critical: Documentation of TopUpAction._calcExchangeAmount

The documentation for _calcExchangeAmount uses the term "underlying" but does not mention the parameter actionToken. Perhaps the documentation could be clarified?

Non-critical: TopUpActionLibrary.lockFunds needs documentation

Function lockFunds spans 32 lines but has no documentation. It's a complicated function and probably deserves some.

Non-critical: No documentation for each field of structs in TopUpAction

It would be very helpful if structs RecordKey, RecordMeta, Record and RecordMeta had more detailed documentation, preferably on a field by field basis.

Non-critical: Inaccurate documentation on TopUpAction.register

The documentation for function register is inaccurate.

It reads:

/**
 * @notice Register a top up action.
 * @dev The `depositAmount` must be greater or equal to the `totalTopUpAmount` (which is denominated in `actionToken`).
 * @param account Account to be topped up (first 20 bytes will typically be the address).
 * @param depositAmount Amount of `depositToken` that will be locked.
 * @param protocol Protocol which holds position to be topped up.
 * @param record containing the data for the position to register
 */

It states that depositAmount is "Amount of depositToken that will be locked" but this is not completely accurate. The actual amount locked is in record.totalTopUpAmount. The amount locked will be equal to record.totalTopAmount (but denominated in the the deposit token).

This is demonstrated in your tests one test_registration.py:283

I would suggest changing the following

     * @dev The `depositAmount` must be greater or equal to the `totalTopUpAmount` (which is denominated in `actionToken`).

to this

     * @dev The `depositAmount`, once converted to units of `actionToken`, must be greater or equal to the `totalTopUpAmount` (which is denominated in `actionToken`).

Non-critical: execute pays fees in action token by swapping funds denominated in position token

Top-up amounts are denominated in the token of the position (which is not always the action token). While this makes complete sense, the design to pay the top-up fees by swapping the position token for action token seems strange. Why not choose a design where users of the TopUpAction contract deposit both the position token and, separately, action token for the fees?

With the present design you must swap the token for Action Token to pay the fees to the Keeper which introduces issues with slippage. The swap to Action Token can fail if there as been too much slippage.

Typographical errors

• AaveHandler.sol:32 "tup-up" -> "top-up"