code-423n4 / 2024-02-spectra-findings

4 stars 2 forks source link

Users can freely set ibtRate without depositing ibt or buying YT. They are assured of a risk-free instant yield when the ibtrate rises and they can potentially drain the entire contract balance with the use of flash loans. #208

Closed c4-bot-10 closed 8 months ago

c4-bot-10 commented 8 months ago

Lines of code

https://github.com/code-423n4/2024-02-spectra/blob/383202d0b84985122fe1ba53cfbbb68f18ba3986/src/tokens/PrincipalToken.sol#L340-L366 https://github.com/code-423n4/2024-02-spectra/blob/383202d0b84985122fe1ba53cfbbb68f18ba3986/src/tokens/PrincipalToken.sol#L385-L391 https://github.com/code-423n4/2024-02-spectra/blob/383202d0b84985122fe1ba53cfbbb68f18ba3986/src/tokens/PrincipalToken.sol#L609-L631 https://github.com/code-423n4/2024-02-spectra/blob/383202d0b84985122fe1ba53cfbbb68f18ba3986/src/tokens/YieldToken.sol#L71-L77 https://github.com/code-423n4/2024-02-spectra/blob/383202d0b84985122fe1ba53cfbbb68f18ba3986/src/libraries/PrincipalTokenUtil.sol#L55-L130

Vulnerability details

Impact

The yield on a YT is calculated by taking the difference between the stored ibtRate of the user and the currenct ibtRate. It is assumed that these rates are only set when the user interacts with the protocol by either depositing ibt/assets or buying YT.

However, the updateYield(address user) function is public and has no access control, allowing anyone to set ibtRateOfUser[_user] whenever they wish.

This allows a strategic user to set the ibtRate whenever it is extremely low at no cost, wait for the ibtRate to recover/increase and then use a flashloan make an instant gigantic profit in one transaction by buying YT, claiming yield and selling the YT. This can be chained for a set of accounts, repeating the same process untill the contract balance is drained.

Proof of Concept

When we look at the updateYield function, we can see that anyone can call the function and set ibtRateOfUser[_user] and ptRateOfUser[_user] to the current market values.

There is no check to assure that the caller has any PT or YT in his possession.

    function updateYield(address _user) public override returns (uint256 updatedUserYieldInIBT) {
        (uint256 _ptRate, uint256 _ibtRate) = _updatePTandIBTRates();

        uint256 _oldIBTRateUser = ibtRateOfUser[_user];
        if (_oldIBTRateUser != _ibtRate) {
            ibtRateOfUser[_user] = _ibtRate;
        }
        uint256 _oldPTRateUser = ptRateOfUser[_user];
        if (_oldPTRateUser != _ptRate) {
            ptRateOfUser[_user] = _ptRate;
        }

        // Check for skipping yield update when the user deposits for the first time or rates decreased to 0.
        if (_oldIBTRateUser != 0) {
            updatedUserYieldInIBT = PrincipalTokenUtil._computeYield(
                _user,
                yieldOfUserInIBT[_user],
                _oldIBTRateUser,
                _ibtRate,
                _oldPTRateUser,
                _ptRate,
                yt
            );
            yieldOfUserInIBT[_user] = updatedUserYieldInIBT;
            emit YieldUpdated(_user, updatedUserYieldInIBT);
        }
    }

For an IBT token with a volatile rate between 1 - 50.

Alice calls updateYield(address Alice) when the ibtRate is 1.

ibtRateOfUser[Alice] = 1.

She then wait until the ibtRate increases to 50 and she utilises a flashloan to buy the maximum amount of YT she can obtain in one transfer. The YieldToken.transfer is called which calls the beforeYtTransfer function.

    function transfer(
        address to,
        uint256 amount
    ) public virtual override(IYieldToken, ERC20Upgradeable) returns (bool success) {
        IPrincipalToken(pt).beforeYtTransfer(msg.sender, to);
        return super.transfer(to, amount);
    }

Here the yield for the buyer and seller is updated. But since Alice already set her ibtRate & ptRate before buying, the _oldIBTRateUse !=0 check is circumvented and _computeYield() is called!

    function beforeYtTransfer(address _from, address _to) external override {
        if (msg.sender != yt) {
            revert UnauthorizedCaller();
        }
        updateYield(_from);
        updateYield(_to);
    }

Here we take the case of only positive yield. The case of positive and negative yield with more positive is also possible, the effects are nearly the same.

userYTBalanceInRay = 1_000_000
_oldIBTRate == ibtRateOfUser[Alice] = 1
_ibtRate = 50

    function _computeYield()
        uint256 newYieldInIBTRay;
        uint256 userYTBalanceInRay = IYieldToken(_yt).actualBalanceOf(_user).toRay(
            IYieldToken(_yt).decimals() 
        );
        // ibtOfPT is the yield generated by each PT corresponding to the YTs that the user holds
        uint256 ibtOfPTInRay = userYTBalanceInRay.mulDiv(_oldPTRate, _oldIBTRate); 
        if (_oldPTRate == _ptRate && _ibtRate > _oldIBTRate) {
            // only positive yield happened
            newYieldInIBTRay = ibtOfPTInRay.mulDiv(_ibtRate - _oldIBTRate, _ibtRate);
            // yield =  1_000_000.mulDiv(50 -1, 50);           
       }

She calls claimYield to safely obtain her profits and she resells the YT she bought.

If Alice sets the ibtRate for a number of accounts (Alice1, Alice2, Alice3, ...), she can effectively drain the entire contract balance by chaining flashloan transactions in the same block.

Tools Used

Manual Review

Recommended Mitigation Steps

The simplest mitigation is to change the visibility of updateYield for public to internal. There is no reason why this function should be able to be called by anyone.

Assessed type

Access Control

c4-pre-sort commented 8 months ago

gzeon-c4 marked the issue as high quality report

c4-pre-sort commented 8 months ago

gzeon-c4 marked the issue as primary issue

gzeon-c4 commented 8 months ago

note that some duplicates did not mention the full attack

c4-sponsor commented 8 months ago

yanisepfl (sponsor) disputed

yanisepfl commented 8 months ago

wait for the ibtRate to recover/increase and then use a flashloan make an instant gigantic profit in one transaction by buying YT, claiming yield and selling the YT

Buying YTs updates the yield for both the buyer and sender, so the yield generated before the buying gets accumulated for the seller. In the mentioned scenario, the claiming yield part would result in claiming 0 for the attacker since between the buying of YT and claiming yield no yield has been generated.

In the PoC, at this step:

Here the yield for the buyer and seller is updated. But since Alice already set her ibtRate & ptRate before buying, the _oldIBTRateUse !=0 check is circumvented and _computeYield() is called! It is assumed that the rate taken into account for Alice is 1, whereas in reality when she bought the YT the rate was at 50, so that would be the one taken into account for Alice. _computeYield() would thus return 0. She calls claimYield to safely obtain her profits and she resells the YT she bought. This would result in obtaining a profit of 0

This issue is wrong and thus disputed.

c4-judge commented 8 months ago

JustDravee marked the issue as unsatisfactory: Invalid

c4-judge commented 8 months ago

JustDravee marked the issue as unsatisfactory: Invalid