A borrower, who does a over-repayment, will lose their "excess" amount of ERC20 to be repaid

[hats-bug-reporter[bot]]

Github username: @0xmuxyz Twitter username: -- Submission hash (on-chain): 0xc147bab03c54e837c56d6692076882146af531ef99fd92f2c3e9724d6f44f3d8 Severity: medium

Description:

Description

When a borrower would payback (repay) their ERC20 loan, the borrower would call the WiseLending#paybackExactShares() or WiseLending#paybackExactAmount(). When the WiseLending#paybackExactShares() or the WiseLending#paybackExactAmount() would be called, the following internal functions would be called based on the following flow inside the either function like this:

The flow of the internal function call: WiseLending#paybackExactShares() or WiseLending#paybackExactAmount() -> WiseLending#_handlePayback() -> MainHelper#_corePayback() -> WiseLowLevelHelper#_decreasePositionMappingValue()

Description:

Within the WiseLending#paybackExactAmount(),

1. The paybackShares would be calculated with a given _amount via the MainHelper#calculateBorrowShares().
2. The payback process would be proceeded with the given _amount and the paybackShares-calculated.

3. The given _amount of ERC20 would be transferred from the borrower's wallet (msg.sender) to the WiseLending contract via the TransferHelper#_safeTransferFrom(). \ https://github.com/hats-finance/Wise-Lending-0xa2ca45d6e249641e595d50d1d9c69c9e3cd22573/blob/7482c9fd1a27629e87a248c818758445fce6101a/contracts/WiseLending.sol#L1159 \ https://github.com/hats-finance/Wise-Lending-0xa2ca45d6e249641e595d50d1d9c69c9e3cd22573/blob/7482c9fd1a27629e87a248c818758445fce6101a/contracts/WiseLending.sol#L1165-L1168 \ https://github.com/hats-finance/Wise-Lending-0xa2ca45d6e249641e595d50d1d9c69c9e3cd22573/blob/7482c9fd1a27629e87a248c818758445fce6101a/contracts/WiseLending.sol#L1181-L1182 \ https://github.com/hats-finance/Wise-Lending-0xa2ca45d6e249641e595d50d1d9c69c9e3cd22573/blob/7482c9fd1a27629e87a248c818758445fce6101a/contracts/WiseLending.sol#L1185-L1189 \

   /**
    * @dev Ability to payback ERC20 loans
    * by providing exact payback amount.
    */
   function paybackExactAmount(
       uint256 _nftId,
       address _poolToken, 
       uint256 _amount ///<------------- @audit
   )
       external
       syncPool(_poolToken)
       returns (uint256)
   {
       uint256 paybackShares = calculateBorrowShares( ///<------------- @audit
           {
               _poolToken: _poolToken,
               _amount: _amount,  ///<------------- @audit
               _maxSharePrice: false
           }
       );
       ...
   
       _handlePayback(
           msg.sender,
           _nftId,
           _poolToken, 
           _amount, ///<------------- @audit
           paybackShares ///<------------- @audit
       );
   
       _safeTransferFrom(
           _poolToken,  
           msg.sender,
           address(this),
           _amount ///<-------- @audit 
       );

Within the WiseLending#paybackExactShares(),

1. The repaymentAmount would be calculated with a given _shares via the MainHelper#paybackAmount().
2. The payback process would be proceeded with the given _shares and the repaymentAmount-calculated.

3. The repaymentAmount-calculated of ERC20 would be transferred from the borrower's wallet (msg.sender) to the WiseLending contract via the TransferHelper#_safeTransferFrom(). \ https://github.com/hats-finance/Wise-Lending-0xa2ca45d6e249641e595d50d1d9c69c9e3cd22573/blob/7482c9fd1a27629e87a248c818758445fce6101a/contracts/WiseLending.sol#L1202 \ https://github.com/hats-finance/Wise-Lending-0xa2ca45d6e249641e595d50d1d9c69c9e3cd22573/blob/7482c9fd1a27629e87a248c818758445fce6101a/contracts/WiseLending.sol#L1208-L1210 \ https://github.com/hats-finance/Wise-Lending-0xa2ca45d6e249641e595d50d1d9c69c9e3cd22573/blob/7482c9fd1a27629e87a248c818758445fce6101a/contracts/WiseLending.sol#L1221 \ https://github.com/hats-finance/Wise-Lending-0xa2ca45d6e249641e595d50d1d9c69c9e3cd22573/blob/7482c9fd1a27629e87a248c818758445fce6101a/contracts/WiseLending.sol#L1225-L1229

   /**
    * @dev Ability to payback ERC20 loans
    * by providing exact payback shares.
    */
   function paybackExactShares(
       uint256 _nftId,
       address _poolToken, 
       uint256 _shares      ///<------------- @audit
   )
       external
       syncPool(_poolToken)
       returns (uint256)
   {
       uint256 repaymentAmount = paybackAmount( ///<------------- @audit
           _poolToken,
           _shares  ///<------------- @audit
       );
       ...
   
       _handlePayback(
           msg.sender,
           _nftId,
           _poolToken, 
           repaymentAmount, ///<------------- @audit
           _shares
       );
   
       _safeTransferFrom( 
           _poolToken,
           msg.sender,
           address(this),
           repaymentAmount ///<------------- @audit
       );

Within the MainHelper#_corePayback(), the WiseLowLevelHelper#_decreasePositionMappingValue() would be called with the userBorrowShares storage and a given _shares like this: \ https://github.com/hats-finance/Wise-Lending-0xa2ca45d6e249641e595d50d1d9c69c9e3cd22573/blob/7482c9fd1a27629e87a248c818758445fce6101a/contracts/MainHelper.sol#L769-L774

    function _corePayback(
        uint256 _nftId,
        address _poolToken,
        uint256 _amount,
        uint256 _shares /// @<-------- audit
    )
        internal
    {
        ...
        _decreasePositionMappingValue(
            userBorrowShares, /// @<-------- audit
            _nftId,
            _poolToken,
            _shares /// @<-------- audit
        );

        if (userBorrowShares[_nftId][_poolToken] > 0) {
            return;
        }
        ...

Within the WiseLowLevelHelper#_decreasePositionMappingValue(), a given amount would be subtracted from a given _nftId and _poolToken of the userMapping (userMapping[_nftId][_poolToken]) like this: \ https://github.com/hats-finance/Wise-Lending-0xa2ca45d6e249641e595d50d1d9c69c9e3cd22573/blob/7482c9fd1a27629e87a248c818758445fce6101a/contracts/WiseLowLevelHelper.sol#L379

    function _decreasePositionMappingValue(
        mapping(uint256 => mapping(address => uint256)) storage userMapping,
        uint256 _nftId,
        address _poolToken,
        uint256 _amount
    )
        internal
    {
        userMapping[_nftId][_poolToken] -= _amount; ///<--------- @audit
    }

(NOTE：On the assumption that, the userBorrowShares is equal to the userMapping would be same (userBorrowShares == userMapping)).

When the WiseLowLevelHelper#_decreasePositionMappingValue() would be called via the MainHelper#_corePayback(), a given amount to be paid back in the WiseLowLevelHelper#_decreasePositionMappingValue() is supposed to be less than or equal to the maximum borrowed-shares (userMapping[_nftId][_poolToken]).

However, within the both functions (the MainHelper#_corePayback() and the WiseLowLevelHelper#_decreasePositionMappingValue()), there is no validation to check whether or not a given amount to be paid back is supposed to be less than or equal to the maximum borrowed-shares (userBorrowShares[_nftId][_poolToken] or userMapping[_nftId][_poolToken]) of the borrower.

This allow a borrower to payback ERC20 loans by providing either exact payback amount or shares via either the WiseLending#paybackExactShares() or WiseLending#paybackExactAmount(), which exceed their maximum borrowed-shares (userBorrowShares[_nftId][_poolToken] or userMapping[_nftId][_poolToken]).

PoC (Scenario)

This is problematic. Because, since the _amount or the repaymentAmount of ERC20 to be paid back would be transferred from the borrower's wallet to the WiseLending contract via the TransferHelper#_safeTransferFrom() when the WiseLending#paybackExactAmount() or the WiseLending#paybackExactShares() would be called, the excess amount of ERC20 to be paid back will be stuck inside the WiseLending contract and it will never be refunded to the borrower's wallet - if a borrower would payback ERC20 loans by providing either exact payback amount or shares, which exceed their maximum borrowed-shares (userBorrowShares[_nftId][_poolToken] or userMapping[_nftId][_poolToken]).

As a result, the borrower would lose the excess amount of ERC20-paid back - due to that their excess amount of ERC20 to be paid back will be stuck inside the WiseLending contract and it will never be refunded to the borrower's wallet.

Recommendation

This can mitigate by either "Recommendation①" or "Recommendation②":

Recommendation①: \ Within the WiseLending#paybackExactShares() and the WiseLending#paybackExactAmount(), consider a logic to refund the excess amount of ERC20 to be paid back.

ref: The refund logic-implemented in the WiseLending#paybackExactAmountETH() seems to be a good reference.

• https://github.com/hats-finance/Wise-Lending-0xa2ca45d6e249641e595d50d1d9c69c9e3cd22573/blob/7482c9fd1a27629e87a248c818758445fce6101a/contracts/WiseLending.sol#L1114-L1128
• https://github.com/hats-finance/Wise-Lending-0xa2ca45d6e249641e595d50d1d9c69c9e3cd22573/blob/7482c9fd1a27629e87a248c818758445fce6101a/contracts/WiseLending.sol#L1142-L1147

Recommendation②: \ Within the MainHelper#_corePayback(), consider adding a validation to check whether or not a given amount to be paid back is supposed to be less than or equal to the maximum borrowed-shares (userBorrowShares[_nftId][_poolToken]) of the borrower like this:

    function _corePayback(
        uint256 _nftId,
        address _poolToken,
        uint256 _amount,
        uint256 _shares
    )
        internal
    {
        ...
+       require(userBorrowShares[_nftId][_poolToken] >= _shares, "A given _shares to be paid back must be less than or equal to the maximum borrowed-shares of the borrower");

        _decreasePositionMappingValue(
            userBorrowShares,
            _nftId,
            _poolToken,
            _shares
        );
         ...
    }

Or,

Within the WiseLowLevelHelper#_decreasePositionMappingValue(), consider adding a validation to check whether or not a given amount to be paid back is supposed to be less than or equal to the maximum borrowed-shares (userMapping[_nftId][_poolToken]) of the borrower like this:

    function _decreasePositionMappingValue(
        mapping(uint256 => mapping(address => uint256)) storage userMapping,
        uint256 _nftId,
        address _poolToken,
        uint256 _amount
    )
        internal
    {
+       require(userMapping[_nftId][_poolToken] >= _amount, "A given _amount to be paid back must be less than or equal to the maximum borrowed-shares of the borrower");

        userMapping[_nftId][_poolToken] -= _amount;
    }

[vm06007]

Hello @0xmuxyz

Thanks for very long explanation for bug consideration, but at the end it comes down to mappings that you've outlined: userBorrowShares[_nftId][_poolToken] or as reference userMapping[_nftId][_poolToken] when _amount of _shares are subtracted from these examples.

Have you considered that if the amount subtracted is greater than the value stored in the mapping, transaction would revert preventing this scenario to go through?

In other words, could you explain why is it necessary to put redundant require check if the default behavior already reverts anyway preventing that scenario you are trying to explain?

I would understand your concern if the decrease is handled like so:

unchecked {
   userMapping[_nftId][_poolToken] -= _amount;
}

but it is not, hence such operation is impossible to subtract more shares than user has, making that check redundant.

[0xmuxyz]

Hi @vm06007, thanks for quickly reviewing my submission!

Sorry, I realized my mistakes including what you pointed out. Thereby, I submitted the fixed version of this submission as another submission (named "Part 2").

So, could you review my another submission above?