0xadrii - Using a wrong interface breaks uAD token incentivized transfers

[sherlock-admin2]

0xadrii

high

Using a wrong interface breaks uAD token incentivized transfers

Summary

Using the wrong interface for incentives will cause all transfers where incentivize() is called to fail.

Vulnerability Detail

The uAD stablecoin incorporates an incentives mechanism that aims at stabilizing the token’s price when it’s value is different from $1.

From the [Ubiquity documentation](https://github.com/ubiquity/ubiquity-dollar/wiki/08.-uAD#incentives-for-traders): “As the token price drops below 1 USD, traders on the 3CRV pool will be incentivized to buy the tokens and disincentivized to sell the token. Incentivization is achieved by burning a percentage of all uAD sold and issuing governance tokens (UBQ) in equal proportions for all uAD bought.”

At a code-level, Ubiquity implements the incentives by overriding the internal ERC20’s _transfer() function, adding an additional _checkAndApplyIncentives() method:

// UbiquityDollarToken.sol
function _transfer(
        address sender,
        address recipient,
        uint256 amount
    ) internal override {
        super._transfer(sender, recipient, amount);
        _checkAndApplyIncentives(sender, recipient, amount);
    }

_checkAndApplyIncentives() will then apply the corresponding incentives each time a transfer of uAD dollar takes place. In order to apply incentives, an incentives contract must be set in an incentiveContract mapping for each actor participating in the uAD transfer. If the actor (sender, recipient, operator or address(0)) has an incentives contract set in the mapping, the incentive’s incentivize() function will be called:

// UbiquityDollarToken.sol
function _checkAndApplyIncentives(
        address sender,
        address recipient,
        uint256 amount
    ) internal {
        // incentive on sender
        address senderIncentive = incentiveContract[sender];
        if (senderIncentive != address(0)) {
            IIncentive(senderIncentive).incentivize(
                sender,
                recipient,
                _msgSender(),
                amount
            );
        }

        // incentive on recipient
        address recipientIncentive = incentiveContract[recipient];
        if (recipientIncentive != address(0)) {
            IIncentive(recipientIncentive).incentivize(
                sender,
                recipient, 
                _msgSender(),
                amount
            );
        }

        // incentive on operator
        address operatorIncentive = incentiveContract[_msgSender()];
        if (
            _msgSender() != sender &&
            _msgSender() != recipient &&
            operatorIncentive != address(0)
        ) {
            IIncentive(operatorIncentive).incentivize(
                sender,
                recipient,
                _msgSender(),
                amount
            );
        }

        // all incentive, if active applies to every transfer
        address allIncentive = incentiveContract[address(0)];
        if (allIncentive != address(0)) {
            IIncentive(allIncentive).incentivize(
                sender,
                recipient,
                _msgSender(),
                amount
            );
        }
    }

The IIncentive.sol interface used in order to call incentivize() is the following:

// IIncentive.sol
interface IIncentive {
    /**
     * @notice Applies incentives on transfer
     * @param sender the sender address of Ubiquity Dollar
     * @param receiver the receiver address of Ubiquity Dollar
     * @param operator the operator (msg.sender) of the transfer
     * @param amount the amount of Ubiquity Dollar transferred
     */
    function incentivize(
        address sender,
        address receiver,
        address operator,
        uint256 amount
    ) external;
}

As we can see, the interface’s incentivize() function has 4 parameters ( sender, receiver, operator andamount . However, the incentivize() function in the expected incentives contract to be used by Ubiquity (CurveDollarIncentiveFacet.sol) only has 3 parameters:

// CurveDollarIncentiveFacet.sol
function incentivize( 
        address sender,
        address receiver,
        uint256 amountIn
    ) external onlyDollarManager {
        LibCurveDollarIncentive.incentivize(sender, receiver, amountIn);
    }

This will make all calls where there is an incentives contract set completely fail. It will also break one of the crucial mechanisms designed to keep uAD’s peg, making uAD susceptible of never recovering peg.

Note: Checking the report, a question might arise from the sponsor: How can this be wrong if tests are passing? The answer is simple: incentive tests found in UbiquityDollarToken.t.sol use a mock Incentives contract that has the incorrect incentivize() function because it inherits from the wrong IIncentive interface used in ubiquity dollar:

// UbiquityDollarToken.t.sol
contract Incentive is IIncentive {
    function incentivize(
        address sender,
        address recipient,
        address operator,
        uint256 amount
    ) public {}
}

contract UbiquityDollarTokenTest is LocalTestHelper {

...

function setUp() public override {
        incentive_addr = address(new Incentive());
        ...
    }
...
function testTransfer_ShouldCallIncentivize_IfValidTransfer() public {
        ...

        dollarToken.setIncentiveContract(mock_sender, incentive_addr);
        dollarToken.setIncentiveContract(mock_recipient, incentive_addr);
        dollarToken.setIncentiveContract(mock_operator, incentive_addr);
        dollarToken.setIncentiveContract(address(0), incentive_addr);
        dollarToken.setIncentiveContract(dollar_addr, incentive_addr);
        vm.stopPrank();

        vm.prank(mock_sender); 
        vm.expectCall(
            incentive_addr,
            abi.encodeWithSelector(
                Incentive.incentivize.selector,
                mock_sender,
                userB,
                mock_sender,
                1
            )
        );
        dollarToken.transfer(userB, 1);

        ...
    }

The PoC attached below illustrates how the test actually fails if the proper incentives contract incentivize() function (the one present in CurveDollarIncentiveFacet.sol) is used.

Impact

High, all incentivized calls will fail, and an important mechanism designed to keep uAD’s peg will be broken, making the stablecoin incapable of keeping its peg.

Proof of Concept

The following Proof of Concept illustrates how all calls with actors having incentivized contracts will fail. In order to execute the PoC, perform the following steps:

1. Change the incentivize() function in the Incentive contract found in UbiquityDollarToken.t.sol so that it does not inherit from the wrong IIncentive interface, making the function selector be the same as CurveDollarIncentiveFacet's incentivize() selector:

// UbiquityDollarToken.t.sol

contract Incentive {
    function incentivize(
        address sender,
        address receiver,
        uint256 amountIn
    ) public {}
}

2. Add the following function in UbiquityDollarToken.t.sol:

function testVuln_allIncentivizedTransfersWillFail() public {
        // Step 1. Mint tokens to mock_sender and set incentives contract to the Incentive contract
        vm.startPrank(admin);

        dollarToken.mint(mock_sender, 100);

        dollarToken.setIncentiveContract(mock_sender, incentive_addr);

        vm.stopPrank();

        // Step 2. Transfer 1 wei of uAD from `mock_sender` to `random`.
        vm.prank(mock_sender); 
        vm.expectRevert();
        dollarToken.transfer(makeAddr("random"), 1);
    }

3. In order to run the test, enter the ubiquity-dollar/packages/contracts folder in your terminal, and run the following command: forge test --mt testVuln_allIncentivizedTransfersWillFail -vvvvv

Code Snippet

https://github.com/sherlock-audit/2023-12-ubiquity/blob/main/ubiquity-dollar/packages/contracts/src/dollar/interfaces/IIncentive.sol#L23-L28

https://github.com/sherlock-audit/2023-12-ubiquity/blob/main/ubiquity-dollar/packages/contracts/src/dollar/core/UbiquityDollarToken.sol#L92-L135

https://github.com/sherlock-audit/2023-12-ubiquity/blob/main/ubiquity-dollar/packages/contracts/src/dollar/facets/CurveDollarIncentiveFacet.sol#L17-L21

Tool used

Manual Review, foundry

Recommendation

Change the IIncentive so that it has the proper function selector, allowing CurveDollarIncentiveFacet's to be called:

interface IIncentive {
    /**
     * @notice Applies incentives on transfer
     * @param sender the sender address of Ubiquity Dollar
     * @param receiver the receiver address of Ubiquity Dollar
-     * @param operator the operator (msg.sender) of the transfer
     * @param amount the amount of Ubiquity Dollar transferred
     */
    function incentivize(
        address sender,
        address receiver,
-        address operator,
        uint256 amount
    ) external;

}

[sherlock-admin2]

1 comment(s) were left on this issue during the judging contest.

auditsea commented:

Makes no sense

[sherlock-admin2]

1 comment(s) were left on this issue during the judging contest.

auditsea commented:

Makes no sense

[nevillehuang]

Invalid, out of scope. Additionally, incentivize is correctly implemented here

[0xadrii]

Escalate Although the CurveDollarIncentiveFacet.sol file is out of scope, the problem of this vulnerability actually relies in the implementation of the IIncentive.sol interface used in UbiquityDollarToken.sol (which IS a file in scope). Although out of scope, CurveDollarIncentiveFacet.sol is a relevant file to understand if UbiquityDollarToken.sol has been correctly implemented or not.

As mentioned in the bug reported, the problem is found in the incentivize() function found in the IIncentive.sol interface , which is the one used in UbiquityDollarToken.sol. As we can see, it is clear that the implementation of the incentivize() function found in the interface has four parameters:

// `IIncentive.sol`
function incentivize(
        address sender,
        address receiver,
        address operator,
        uint256 amount
    ) external;

However, the incentivize() function expected to be called in the CurveDollarIncentiveFacet.sol has three parameters:

// `CurveDollarIncentiveFacet.sol`
function incentivize( 
        address sender,
        address receiver,
        uint256 amountIn
    ) external onlyDollarManager {
        LibCurveDollarIncentive.incentivize(sender, receiver, amountIn);
    }

This will make all the calls fail because of using a wrong interface.

Although the judge mentioned that the interface is correctly implemented in a comment from some days ago, the report clearly shows that the interface is NOT correct. As you can see in the PoC in my bug report, the code reverts when using the correct implementation of the interface because as mentioned in the report, even the test suite uses WRONG mock contracts to test the incentives functionality.

Regarding the out of scope argument, I believe this is an issue concerning files in scope. It is clear that the interface used in UbiquityDollarToken.sol (a file in scope) is intended to be used to call the incentivize() function in CurveDollarIncentiveFacet.sol, and it is also clear that such interface is actually wrong and will not properly allow interaction with CurveDollarIncentiveFacet.sol's incentivize() function

As a final comment, I'd like to highlight the fact that interfaces should comply to the code they want to interact with, not the other way around. If the CurveDollarIncentiveFacet.sol explicitly shows that incentivize() should be called with THREE arguments, there is no justification for the incentivize() function in IIncentive.sol to have FOUR arguments.

[sherlock-admin2]

Escalate Although the CurveDollarIncentiveFacet.sol file is out of scope, the problem of this vulnerability actually relies in the implementation of the IIncentive.sol interface used in UbiquityDollarToken.sol (which IS a file in scope). Although out of scope, CurveDollarIncentiveFacet.sol is a relevant file to understand if UbiquityDollarToken.sol has been correctly implemented or not.

As mentioned in the bug reported, the problem is found in the incentivize() function found in the IIncentive.sol interface , which is the one used in UbiquityDollarToken.sol. As we can see, it is clear that the implementation of the incentivize() function found in the interface has four parameters:

// `IIncentive.sol`
function incentivize(
        address sender,
        address receiver,
        address operator,
        uint256 amount
    ) external;

However, the incentivize() function expected to be called in the CurveDollarIncentiveFacet.sol has three parameters:

// `CurveDollarIncentiveFacet.sol`
function incentivize( 
        address sender,
        address receiver,
        uint256 amountIn
    ) external onlyDollarManager {
        LibCurveDollarIncentive.incentivize(sender, receiver, amountIn);
    }

This will make all the calls fail because of using a wrong interface.

Although the judge mentioned that the interface is correctly implemented in a comment from some days ago, the report clearly shows that the interface is NOT correct. As you can see in the PoC in my bug report, the code reverts when using the correct implementation of the interface because as mentioned in the report, even the test suite uses WRONG mock contracts to test the incentives functionality.

Regarding the out of scope argument, I believe this is an issue concerning files in scope. It is clear that the interface used in UbiquityDollarToken.sol (a file in scope) is intended to be used to call the incentivize() function in CurveDollarIncentiveFacet.sol, and it is also clear that such interface is actually wrong and will not properly allow interaction with CurveDollarIncentiveFacet.sol's incentivize() function

As a final comment, I'd like to highlight the fact that interfaces should comply to the code they want to interact with, not the other way around. If the CurveDollarIncentiveFacet.sol explicitly shows that incentivize() should be called with THREE arguments, there is no justification for the incentivize() function in IIncentive.sol to have FOUR arguments.

You've created a valid escalation!

To remove the escalation from consideration: Delete your comment.

You may delete or edit your escalation comment anytime before the 48-hour escalation window closes. After that, the escalation becomes final.

[nevillehuang]

Since the affected contracts IIncentive.sol and CurveDollarIncentiveFacet.sol is out of scope, this is invalid based on sherlock scoping rules. I believe no further discussions is required from my side.

1. If a contract is in contest Scope, then all its parent contracts are included by default.
2. In case the vulnerability exists in a library and an in-scope contract uses it and is affected by this bug this is a valid issue. > 3. If there is a vulnerability in a contract from the contest repository but is not included in the scope then issues related to it cannot be considered valid.

[Evert0x]

Planning to reject escalation because of the reasons above

[Evert0x]

Result: Invalid Unique

[sherlock-admin2]

Escalations have been resolved successfully!

Escalation status:

• 0xadrii: rejected