Fujicracy / fuji-v2

Cross-chain money market aggregator
https://fuji-v2-frontend.vercel.app
15 stars 10 forks source link

borrowingVault: owner and receiver of `gainedShares` addresses in liquidate function #347

Closed masaun closed 1 year ago

masaun commented 1 year ago

Title

A user who has the LIQUIDATOR_ROLE will not lose their gainedShares even if their debt would be liquidated

Affected smart contract

Description

Within the BorrowingVault# liquidate(), the owner of debt and the receiver who is a liquidator would be assigned as parameters. Once liquidation occur, the gainedShares will be burned from the owner and the gainedShares will be minted to the receiver like this: https://github.com/Fujicracy/fuji-v2/blob/main/packages/protocol/src/vaults/borrowing/BorrowingVault.sol#L585-L586 https://github.com/Fujicracy/fuji-v2/blob/main/packages/protocol/src/vaults/borrowing/BorrowingVault.sol#L622-L623

  function liquidate(
    address owner,  /// @audit
    address receiver  /// @audit
  )
    external
    hasRole(msg.sender, LIQUIDATOR_ROLE)
    returns (uint256 gainedShares)
  {
    ...

    // Internal share adjusment between 'owner' and 'liquidator'.
    _burn(owner, gainedShares);  /// @audit
    _mint(receiver, gainedShares);  /// @audit

    ...
}

In the case above, a user who has the LIQUIDATOR_ROLE can assign both the owner and the receiver as argument value when the user call the BorrowingVault# liquidate().

The owner and the receiver are supposed to be different address. Because if the same address would be assigned into the both owner and receiver, the gainedShares can be minted to the receiver even if the gainedShares would be burned from the owner.

However, there is no validation to check whether the owner and receiver are different address. If a user who has the LIQUIDATOR_ROLE assign the same address with the owner into the receiver as argument value when the user call the BorrowingVault# liquidate(), the address assigned will not lose their gainedShares even if the owner is liquidated.

Attack scenario

  1. A user who has the LIQUIDATOR_ROLE borrow some amount of tokens based on shares and the user become the owner of the debt.
  2. Once liquidationFactor meet the threshold of liquidation, the debt of the owner will be liquidated.
  3. The user who has the LIQUIDATOR_ROLE call the BorrowingVault# liquidate() by assigning the same address with owner into the receiver.
  4. Once the BorrowingVault# liquidate() is called above, the amount of gainedShares would be burned from the owner then the same amount of the gainedShares would be minted to the receiver.
  5. Assuming that the same address with owner was assigned into the receiver, the owner will not lose their gainedShares. Because the receiver receive the amount of gainedShares burned from the owner.

Recommendation

Consider adding a validation in order to check whether the owner and receiver are different address to the BorrowingVault# liquidate() like this:

  function liquidate(
    address owner,  /// @audit
    address receiver  /// @audit
  )
    /// @audit - Consider adding a validation like this:
+   require(owner != receiver, "Owner and receiver must be different address");
    ...

    // Internal share adjusment between 'owner' and 'liquidator'.
    _burn(owner, gainedShares); 
    _mint(receiver, gainedShares);

    ...
}
0xdcota commented 1 year ago

WontDo: The rationale about the vulnerability is unclear. The liquidator or msg.sender/caller of the liquidate() function, can assign receiver anybody they think should receive the gained shares. If they send them back to the owner, they effectively just gave them "free money". However, this is the choice of the caller, and doing the aforementioned is not profitable. Though, the team does not consider this should be blocked. The original intent is for the liquidator (typically an EOA) assigns a separate multisig or "gnosisSafe" to receive the gainedShares.