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

[hats-bug-reporter[bot]]

Github username: @0xmuxyz Twitter username: -- Submission hash (on-chain): 0x04e9ab7eb52d5abf31c2afc9bf1449a266fb8d29a3316d9f9321eb73e1066535 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#paybackExactAmount() or WiseLending#paybackExactShares() -> 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().

   /**
    * @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().

   /**
    * @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 WiseLending#_handlePayback(), the MainHelper#_corePayback() would be called with a given _amountand _shares like this:

    function _handlePayback(
        address _caller,
        uint256 _nftId,
        address _poolToken,
        uint256 _amount, ///<------------- @audit
        uint256 _shares   ///<------------- @audit
    )
        private
    {
        _corePayback(
            _nftId,
            _poolToken,
            _amount, ///<------------- @audit
            _shares   ///<------------- @audit
        );

Within the MainHelper#_corePayback(), the WiseLowLevelHelper#_decreasePositionMappingValue() would be called with the userBorrowShares storage and a given _shares like this:

    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:

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

(NOTE：The userMapping is equal to the userBorrowShares would be same (`userMapping == userBorrowShares)).

However, in case of the current implementations, there are problems like this:

• When the WiseLending#paybackExactAmount() would be called, a given _amount of ERC20 to be paid back would be transferred to the WiseLending contract as it is - even if the paybackShares, which is calculated based on the given _amount, would exceed the borrower's maxBorrowShares (userBorrowShares[_nftId][_poolToken]).

• When the WiseLending#paybackExactShares() would be called, the repaymentAmount of ERC20 to be paid back, which is calculated based on the given _shares, would be transferred to the WiseLending contract as it is - even if the given _shares would exceed the borrower's maxBorrowShares (userBorrowShares[_nftId][_poolToken]).

Because, in the both cases above, the transaction would not be reverted by any internal functions that is called inside the WiseLending#paybackExactAmount() and WiseLending#paybackExactShares(). (i.e. The MainHelper#_corePayback(), the WiseLowLevelHelper#_decreasePositionMappingValue(), etc)

This lead to that the borrower would lose the excess amount of ERC20 to be 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.

PoC (Scenario)

Assuming that Bob is a borrower.

Scenario①: The WiseLending#paybackExactAmount() would be used for a payback:

• 0/ Let's say Bob's maxBorrowShares (userBorrowShares[_nftId][_poolToken]) would be 150.

• 1/ Bob would call the WiseLending#paybackExactAmount() with the _amount = 100 as the input value.

  • At this point, let's say the paybackShares, which is calculated based on the given _amount (100), would be 200.

• 2/ Although the paybackShares (200) would exceed the Bob's maxBorrowShares (150), the transaction would not be reverted.

• 3/ As a result, the given _amount (100), would be transferred from Bob to the WiseLending contract as it is - although the amount to be transferred is supposed to be less than the given _amount (100). Hence, this is his over-repayment.

  • https://github.com/hats-finance/Wise-Lending-0xa2ca45d6e249641e595d50d1d9c69c9e3cd22573/blob/7482c9fd1a27629e87a248c818758445fce6101a/contracts/WiseLending.sol#L1185-L1190

• 4/ Although Bob did a over-repayment, his over-repaid amount will never be refunded to him and it will be stuck inside the WiseLending contract.

  • As a result, Bob would lose the amount of ERC20, which he did over-repayment.

Scenario②: The WiseLending#paybackExactShares()would be used for a payback:

• 0/ Let's say Bob's maxBorrowShares (userBorrowShares[_nftId][_poolToken]) would be 150.

• 1/ Bob would call the WiseLending#paybackExactShares() with the _shares = 200 as the input value.

  • At this point, let's say the repaymentAmount, which is calculated based on the given _shares (200), would be 100.

• 2/ Although the given _shares (200) would exceed the Bob's maxBorrowShares (150), the transaction would not be reverted.

• 3/ As a result, the repaymentAmount (100), would be transferred from Bob to the WiseLending contract as it is - although the amount to be transferred is supposed to be less than the repaymentAmount (100). Hence, this is his over-repayment.

  • https://github.com/hats-finance/Wise-Lending-0xa2ca45d6e249641e595d50d1d9c69c9e3cd22573/blob/7482c9fd1a27629e87a248c818758445fce6101a/contracts/WiseLending.sol#L1225-L1230

• 4/ Although Bob did a over-repayment, his over-repaid amount will never be refunded to him and it will be stuck inside the WiseLending contract.

  • As a result, Bob would lose the amount of ERC20, which he did over-repayment.

Recommendation

Within the WiseLending#paybackExactAmount(), consider adding a validation to check whether or not the paybackShares is less than or equal to the borrower's maxBorrowShares (userBorrowShares[_nftId][_poolToken]) like this:

    function paybackExactAmount(
        uint256 _nftId,
        address _poolToken, 
        uint256 _amount 
    )
        external
        syncPool(_poolToken)
        returns (uint256)
    {
+       uint256 maxBorrowShares = userBorrowShares[_nftId][_poolToken];

        uint256 paybackShares = calculateBorrowShares( 
            {
                _poolToken: _poolToken,
                _amount: _amount,  
                _maxSharePrice: false
            }
        );

+       require(paybackShares <= maxBorrowShares, "The paybackShares must be less than or equal to the maxBorrowShares");
        ...

        _handlePayback(
            msg.sender,
            _nftId,
            _poolToken, 
            _amount, 
            paybackShares 
        );

        _safeTransferFrom(
            _poolToken,  
            msg.sender,
            address(this),
            _amount 
        );

And,

Within the WiseLending#paybackExactShares, consider adding a validation to check whether or not a given _shares is less than or equal to the borrower's maxBorrowShares (userBorrowShares[_nftId][_poolToken]) like this:

    function paybackExactShares(
        uint256 _nftId,
        address _poolToken, 
        uint256 _shares 
    )
        external
        syncPool(_poolToken)
        returns (uint256)
    {
+       uint256 maxBorrowShares = userBorrowShares[_nftId][_poolToken];
+       require(_shares <= maxBorrowShares, "The _shares to be paid back must be less than or equal to the maxBorrowShares");

        uint256 repaymentAmount = paybackAmount( 
            _poolToken,
            _shares 
        );
        ...

        _handlePayback(
            msg.sender,
            _nftId,
            _poolToken, 
            repaymentAmount, 
            _shares
        );

        _safeTransferFrom( 
            _poolToken,
            msg.sender,
            address(this),
            repaymentAmount 
        );

[0xmuxyz]

@vm06007

This is the fixed version of the my previous submission.

And I would like to add something a little bit for what you pointed out in my previous submission.

Within the code of the WiseLowLevelHelper#_decreasePositionMappingValue() below, if the amount > userMapping[_nftId][_poolToken], the 0 would be stored into the userMapping[_nftId][_poolToken] due to underflow.

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

However, in the fixed version of my submission above, the key point is that the transaction itself would not be reverted and it can keep going - although the 0 would be stored into the userMapping[_nftId][_poolToken] due to underflow. (NOTE：For the userBorrowShares[_nftId][_poolToken], the same situation happen)

As a result, in the case that a borrower would call the WiseLending#paybackExactAmount() with a give _amount (as an input value), the given _amount would be transferred from the borrower to the WiseLending contract as it is - even if the paybackShares, which is calculated based on the given _amount, would exceed the borrower's maxBorrowShares (userBorrowShares[_nftId][_poolToken]).

[vonMangoldt]

There is no unchecked here @vm06007 was pointing out that that would need to be the case for your claim to be valid:

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

This will fail here

[vm06007]

@vm06007

This is the fixed version of the my previous submission.

And I would like to add something a little bit for what you pointed out in my previous submission.

Within the code of the WiseLowLevelHelper#_decreasePositionMappingValue() below, if the amount > userMapping[_nftId][_poolToken], the 0 would be stored into the userMapping[_nftId][_poolToken] due to underflow.

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

However, in the fixed version of my submission above, the key point is that the transaction itself would not be reverted and it can keep going - although the 0 would be stored into the userMapping[_nftId][_poolToken] due to underflow. (NOTE：For the userBorrowShares[_nftId][_poolToken], the same situation happen)

As a result, in the case that a borrower would call the WiseLending#paybackExactAmount() with a give _amount (as an input value), the given _amount would be transferred from the borrower to the WiseLending contract as it is - even if the paybackShares, which is calculated based on the given _amount, would exceed the borrower's maxBorrowShares (userBorrowShares[_nftId][_poolToken]).

@0xmuxyz This is where your assumption and understanding is wrong, 0 won't be stored in the mapping, if you try to subtract value higher than is currently in the mapping (even if its 0 already) transaction will ALWAYS fail. I would suggest to brush your solidity knowledge:

Here is an example:

mapping[_key] = 0;
mapping[_key] -= 1; // will fail and revert;

mapping[_key] = 10;
mapping[_key] -= 12; // will fail and revert;

here you can play with this contract to understand where your assumption is wrong:

contract MappingClass {

    mapping(uint256 => mapping(address => uint256)) public mainMapping;

    function testDecreaseValue(
        uint256 _nftId,
        address _tokenAddress,
        uint256 _amountToSubtract
    )
        external 
    {
        _decreasePositionMappingValue(
            mainMapping,
            _nftId,
            _tokenAddress,
            _amountToSubtract
        );
    }

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

    function testIncreaseValue(
        uint256 _nftId,
        address _tokenAddress,
        uint256 _amountToSubtract
    )
        external 
    {
        _increasePositionMappingValue(
            mainMapping,
            _nftId,
            _tokenAddress,
            _amountToSubtract
        );
    }

    function _increasePositionMappingValue(
        mapping(uint256 => mapping(address => uint256)) storage userMapping,
        uint256 _nftId,
        address _poolToken,
        uint256 _amount
    )
        internal
    {
        userMapping[_nftId][_poolToken] += _amount;
    }
}

1) call testIncreaseValue(10, 0x5B38Da6a701c568545dCfcB03FcB875f56beddC4, 100); 2) call testDecreaseValue(10, 0x5B38Da6a701c568545dCfcB03FcB875f56beddC4, 123);

observe transaction fails.