KupiaSec - Invalid Redstone oracle payload size prevents the protocol from working properly

[sherlock-admin4]

KupiaSec

High

Invalid Redstone oracle payload size prevents the protocol from working properly

Summary

In api contract, it uses 224 bytes as maximum length for Redstone's oracle payload, but oracle price data and signatures of 3 signers exceeds 225 bytes thus reverting transactions.

Vulnerability Detail

In every external function of api contract, it uses 224 bytes as maximum size for Redstone oracle payload.

However, the RedstoneExtractor requires oracle data from at least 3 unique signers, as implemented in PrimaryProdDataServiceConsumerBase contract. Each signer needs to send token price information like token identifier, price, timestamp, etc and 65 bytes of signature data.
Just with basic calculation, the oracle payload size exceeds 224 bytes.

Here's some proof of how Redstone oracle data is used:

• Check one of transactions from here that uses Redstone oracle.
• One of transaction is this one on Avalanche, which has 9571 bytes of data.
• Check this Blocksec Explorer, and it also shows the oracle data of 3 signers are passed.

As shown from the proof above, the payload size of Redstone data is huge, so setting 224 bytes as upperbound reverts transactions.

Impact

Protocol does not work because the payload array size limit is too small.

Code Snippet

https://github.com/sherlock-audit/2024-08-velar-artha/blob/18ef2d8dc0162aca79bd71710f08a3c18c94a36e/gl-sherlock/contracts/api.vy#L83
https://github.com/sherlock-audit/2024-08-velar-artha/blob/18ef2d8dc0162aca79bd71710f08a3c18c94a36e/gl-sherlock/contracts/api.vy#L58

Tool used

Manual Review

Recommendation

The upperbound size of payload array should be increased to satisfy Redstone oracle payload size.

[KupiaSecAdmin]

Escalate

Information required for this issue to be rejected.

[sherlock-admin3]

Escalate

Information required for this issue to be rejected.

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.

[WangSecurity]

@KupiaSecAdmin, could you help me understand how did you get the 9571 data size? can't see it in the links, but I assume I'm missing it somewhere.

[KupiaSecAdmin]

@WangSecurity - The data size is fetched from this transaction which uses RedStone oracle.
The calldata size of the transaction is 9574 bytes, but the function receives 2 variables which sums up 64 bytes, so remaining bytes 9574 - 64 = 9510 bytes of data is for RedStone oracle payload.

Maybe, it includes some additional oracle data, but the main point is that oracle price data of 3 oracles can't fit in 224 bytes. Because each oracle signature is 65 bytes which means it has 195 bytes of signature, and also the oracle data should include price, timestamp, and token name basically. So it never fits in 224 bytes.

[rickkk137]

i used minimal foundry package to generate some payload

bytes memory redstonePayload0 = getRedstonePayload("BTC:120:8,ETH:69:8");
//result length 440 chars
425443000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000002cb4178004554480000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000019b45a50001921f3ed12d000000200000022af29790252e02178ff033567c55b4145e48cbec0434729f363a01675d9a88d618583b722a092e3e6d9944b69c79cadf793b2950f1a99fd595933609984ef1c01c0001000000000002ed57011e0000
 bytes memory redstonePayload = getRedstonePayload("BTC:120:8");
//result length 312 chars
425443000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000002cb41780001921f3ed1d400000020000001e3cabe40b42498dccea014557946f3bae4d29783c9dc7deb499c5a2d6d1901412cba9924d1c1fdfc429c07361ed9fab2789e191791a31ecdbbd77ab95a373f491c0001000000000002ed57011e0000

def mint(
  base_token  : address, #ERC20
  quote_token : address, #ERC20
  lp_token    : address, #ERC20Plus
  base_amt    : uint256,
  quote_amt   : uint256,
  desired     : uint256,
  slippage    : uint256,
 @>>> payload     : Bytes[224]

payload parameter's length is 224 its mean we can pass a string with max length 448 character ,hence we can pass above payload to api functions,I want to say length of payload depend on number of symbols which we pass to get payload and max size for 2 symbol is 440 character

[KupiaSecAdmin]

@rickkk137 - Bytes type is different from strings as documented here, Bytes[224] means 224 bytes.

[rickkk137]

Each pair of hexadecimal digits represents one byte and in above examples first example's length is [440 / 2] 220 bytes and second one's length is [312/2] 156 bytes, further more in redstoneExtractor contract developer just uses index 0 which its mean requestRedstonePayload function just get one symbol as a parameter in Velar

[KupiaSecAdmin]

@rickkk137 - Seems you misunderstand something here.
You generated Redstone oracle payload using their example repo that results in 220 bytes, this is the oracle data of one signer. For Verla, it requires oracle data of 3 signers so that it can take median price of 3 prices.

And I agree with the statement that it uses one symbol, yes Verla only requires BTC price from Redstone oracle.

[rickkk137]

Data is packed into a message according to the following structure

The data signature is verified by checking if the signer is one of the approved providers

its mean just one sign there is in every payload

Symbol	Value	Timestamp	Size(n)	Signature
32b	32b	32b	1b	65b

max size for 1 symbol: 32 + 32 + 32 + 1 + 65 = 172 bytes
https://github.com/redstone-finance/redstone-evm-connector

function getAuthorisedSignerIndex(
    address signerAddress
  ) public view virtual override returns (uint8) {
    if (signerAddress == 0x8BB8F32Df04c8b654987DAaeD53D6B6091e3B774) {
      return 0;
    } else if (signerAddress == 0xdEB22f54738d54976C4c0fe5ce6d408E40d88499) {
      return 1;
    } else if (signerAddress == 0x51Ce04Be4b3E32572C4Ec9135221d0691Ba7d202) {
      return 2;
    } else if (signerAddress == 0xDD682daEC5A90dD295d14DA4b0bec9281017b5bE) {
      return 3;
    } else if (signerAddress == 0x71d00abE308806A3bF66cE05CF205186B0059503) {
      return 4;
    } else {
      revert SignerNotAuthorised(signerAddress);
    }
  }

[WangSecurity]

@KupiaSecAdmin just a small clarification, the idea that you need 3 signers in payload is based on which documentation?

[KupiaSecAdmin]

@WangSecurity - It comes from the RedStone implementation that Verla uses: https://github.com/redstone-finance/redstone-oracles-monorepo/blob/2bbf16cbbaa36f7046034dbbd968f3673a0657e8/packages/evm-connector/contracts/data-services/PrimaryProdDataServiceConsumerBase.sol#L12-L14

And you know, usually, using one signer data as oracle causes issue because its data can be malicious, that's how the protocol takes 3 signers and take median price among them.

[WangSecurity]

Unfortunately, I'm still not convinced enough this is actually a valid finding. Firstly, the transaction linked before, which has 9574 bytes size of calldata and uses the RedStone oracle, doesn't have the Redstone's payload as an input parameter as Velar does it.
Secondly, this transaction is on Avalanche, while Velar will be deployed on Bob.

Hence, this is not a sufficient argument that 224 Bytes won't be enough.
Thirdly, payload is used when calling the extract_price function which doesn't even use that payload. Hence, I don't see a sufficient argument for this being a medium, but before making the decision, I'm giving some time to correct my points.

[rickkk137]

@WangSecurity u can pass n asset to fetchPayload function and that isn't const its mean payload length is flexible which depend on protocol and when we look at extract_price which just uses index 0 its mean they are suppose to pass just one symbol to redstone function to get payload and base on redstone document payload's length just for one symbol is 172 bytes which is less than 224 bytes

payload_size = n*(32 + 32) + 32 + 1 + 65
payload_size_for_one_asset = 1 * (32 + 32) + 32 + 1 + 65 = 172 bytes
payload_size_for_two_asset = 2 * (32 + 32) + 32 + 1 + 65 = 226 bytes
payload_size_for_three_asset = 3 * (32 + 32) + 32 + 1 + 65 = 290 bytes
...

[KupiaSecAdmin]

@WangSecurity - The 9574 bytes of payload is one example of Redstone payload.

The point is that it's obvious the price data can't fit in 224 bytes. As @rickkk137 mentioned, payload size for one asset of one signer is 172 bytes, but Verla requires oracle data of 3 signers, which will result in >500 bytes.

[rickkk137]

Velar protocol uses version 0.6.1 redstone-evm-connector and in this version RedstoneConsumerBase::getUniqueSignersThreshold returns 1 in this path / @redstone-finance/evm-connector / contracts / core / RedstoneConsumerBase.sol,hence just one signer is required
https://www.npmjs.com/package/@redstone-finance/evm-connector/v/0.6.1?activeTab=code
also when we look at make file we realized Velar's developers directly copy RedstoneConsumerBase contract without any changes,furthermore usingDataService function get unique signer as a parameter and when they restricted payload to 224 bytes its mean they want to pass 1 as a uniqueSignersCount

 const wrappedContract = WrapperBuilder.wrap(contract).usingDataService({
    dataServiceId: "redstone-rapid-demo",
 @>>>   uniqueSignersCount: 1,
    dataFeeds: ["BTC", "ETH", "BNB", "AR", "AVAX", "CELO"],
  });

https://github.com/redstone-finance/redstone-showroom/blob/0db580be39bdccb9632ee4d8d8c80e4182d8e266/example/getTokensPrices.ts#L22

[KupiaSecAdmin]

@rickkk137 - Check RedstoneExtractor.sol, the contract inherits from PrimaryProdDataServiceConsumerBase contract which is located in "./vendor/data-services/PrimaryProdDataServiceConsumerBase.sol" that is copied after make, which returns 3 as number of signers.

[WangSecurity]

As I understand, @KupiaSecAdmin is indeed correct here, and the current payload size won't work when the contracts are deployed on the Live chain. After clarifying with the sponsor, they've said they used 224 only for testnet and will increase it for the live chain, but there's no info about it in README or code comments. Hence, this should be indeed a valid issue. Planning to accept the escalation and validate with Medium severity. Medium severity because there is no loss of funds, and the second definition of High severity excludes contracts not working:

Inflicts serious non-material losses (doesn't include contract simply not working).

Hence, medium severity is appropriate:

Breaks core contract functionality, rendering the contract useless or leading to loss of funds.

Are there any duplicates?

[KupiaSecAdmin]

@WangSecurity - Agree with having this as Medium severity. Thanks for your confirmation.

[WangSecurity]

Result:
Medium
Unique

[sherlock-admin4]

Escalations have been resolved successfully!

Escalation status:

• KupiaSecAdmin: accepted