Revolution Protocol - adeolu's results

Lines of code

https://github.com/code-423n4/2023-12-revolutionprotocol/blob/d42cc62b873a1b2b44f57310f9d4bbfdd875e8d6/packages/revolution/src/ERC20TokenEmitter.sol#L152-L156

Vulnerability details

Impact

in buyToken(), users can use ether to buy ERC20TokenEmitter tokens based on the current quote price returned by getTokenQuoteForEther(). The price amount returned by getTokenQuoteForEther() is calculated based on the VRGDA contract yTox() fcn. We can compare the function of the calc in yTox() to the x =y*k calc majorly used in many AMMs when exchanging one asset for another. In this case the yTox() returned amount is the output amount for when we buy tokens with ether via buyTokens() .

The returned amount of yTox() does not always represent a 1 to 1 swap so it can be possible for the output amount to be an undesired value. The current implementation doesnt check for that and does not allow users to supply their minimum expected output amount. In deFi it is always expected that when exchanging one asset for the other especially when the two assets are of different values/prices, we must allow for specification of the trade's minimum output amount so as to prevent undesired trade outcomes.

Proof of Concept

• in buyTokens(), user sends in ether to make the purchase, this is enforced here https://github.com/code-423n4/2023-12-revolutionprotocol/blob/d42cc62b873a1b2b44f57310f9d4bbfdd875e8d6/packages/revolution/src/ERC20TokenEmitter.sol#L160

        require(msg.value > 0, "Must send ether");

• after ether value is gotten, the total tokens to give to buyers for the ether amount sent (totalTokensForBuyer) is calculated via getTokenQuoteForEther()

https://github.com/code-423n4/2023-12-revolutionprotocol/blob/d42cc62b873a1b2b44f57310f9d4bbfdd875e8d6/packages/revolution/src/ERC20TokenEmitter.sol#L184

        int totalTokensForBuyers = toPayTreasury > 0 ? getTokenQuoteForEther(toPayTreasury) : int(0);

• in getTokenQuoteForEther() we can see the use of the VRGDA function, yToX(). https://github.com/code-423n4/2023-12-revolutionprotocol/blob/d42cc62b873a1b2b44f57310f9d4bbfdd875e8d6/packages/revolution/src/ERC20TokenEmitter.sol#L254C1-L265C1

    function getTokenQuoteForEther(uint256 etherAmount) public view returns (int gainedX) {
        require(etherAmount > 0, "Ether amount must be greater than 0");
        // Note: By using toDaysWadUnsafe(block.timestamp - startTime) we are establishing that 1 "unit of time" is 1 day.
        // solhint-disable-next-line not-rely-on-time
        return
            vrgdac.yToX({
                timeSinceStart: toDaysWadUnsafe(block.timestamp - startTime),
                sold: emittedTokenWad,
                amount: int(etherAmount)
            });
    }

• finally we see totalTokensForBuyer being used in the payment of tokens to the buyers for the ether sent and the purchase is finalised.

https://github.com/code-423n4/2023-12-revolutionprotocol/blob/d42cc62b873a1b2b44f57310f9d4bbfdd875e8d6/packages/revolution/src/ERC20TokenEmitter.sol#L207C1-L216C1

        //Mint tokens to buyers

        for (uint256 i = 0; i < addresses.length; i++) {
            if (totalTokensForBuyers > 0) {
                // transfer tokens to address
                _mint(addresses[i], uint256((totalTokensForBuyers * int(basisPointSplits[i])) / 10_000));
            }
            bpsSum += basisPointSplits[i];
        }

All through these steps, there is no check to see if the totalTokensForBuyers is a desired output amout, if its lower than user expected. Lack of this validation can cause the user to lose assets or sometimes take on unforseen risks.

Tools Used

manual review

Recommended Mitigation Steps

add slippage check to the buyToken() fcn, allow users to specify a minimum Output amount for the token amount they will be getting in return for paying ether.

Assessed type

Other