Skip to content

Commit b0a2b13

Browse files
ali-behjatiali-behjati
authored
[eth] Wrap parsing method with unchecked (#378)
* Move parsing method to unchecked There is no overflow possible there * Update another for loop to be unchecked
1 parent 8db2af6 commit b0a2b13

File tree

1 file changed

+133
-121
lines changed

1 file changed

+133
-121
lines changed

‎ethereum/contracts/pyth/Pyth.sol

Lines changed: 133 additions & 121 deletions
Original file line numberDiff line numberDiff line change
@@ -37,8 +37,10 @@ abstract contract Pyth is PythGetters, PythSetters, AbstractPyth {
3737
uint requiredFee = getUpdateFee(updateData);
3838
require(msg.value >= requiredFee, "insufficient paid fee amount");
3939

40-
for(uint i = 0; i < updateData.length; i++) {
40+
for(uint i = 0; i < updateData.length; ) {
4141
updatePriceBatchFromVm(updateData[i]);
42+
43+
unchecked { i++; }
4244
}
4345

4446
emit UpdatePriceFeeds(msg.sender, updateData.length, requiredFee);
@@ -58,158 +60,168 @@ abstract contract Pyth is PythGetters, PythSetters, AbstractPyth {
5860
}
5961

6062
function parseAndProcessBatchPriceAttestation(IWormhole.VM memory vm) internal {
61-
bytes memory encoded = vm.payload;
62-
uint index = 0;
63-
64-
// Check header
65-
{
66-
uint32 magic = encoded.toUint32(index);
67-
index += 4;
68-
require(magic == 0x50325748, "invalid magic value");
63+
// Most of the math operations below are simple additions.
64+
// In the places that there is more complex operation there is
65+
// a comment explaining why it is safe. Also, byteslib
66+
// operations have proper require.
67+
unchecked {
68+
bytes memory encoded = vm.payload;
69+
uint index = 0;
70+
71+
// Check header
72+
{
73+
uint32 magic = encoded.toUint32(index);
74+
index += 4;
75+
require(magic == 0x50325748, "invalid magic value");
76+
77+
uint16 versionMajor = encoded.toUint16(index);
78+
index += 2;
79+
require(versionMajor == 3, "invalid version major, expected 3");
80+
81+
uint16 versionMinor = encoded.toUint16(index);
82+
index += 2;
83+
require(versionMinor >= 0, "invalid version minor, expected 0 or more");
84+
85+
uint16 hdrSize = encoded.toUint16(index);
86+
index += 2;
87+
88+
// NOTE(2022-04-19): Currently, only payloadId comes after
89+
// hdrSize. Future extra header fields must be read using a
90+
// separate offset to respect hdrSize, i.e.:
91+
//
92+
// uint hdrIndex = 0;
93+
// bpa.header.payloadId = encoded.toUint8(index + hdrIndex);
94+
// hdrIndex += 1;
95+
//
96+
// bpa.header.someNewField = encoded.toUint32(index + hdrIndex);
97+
// hdrIndex += 4;
98+
//
99+
// // Skip remaining unknown header bytes
100+
// index += bpa.header.hdrSize;
101+
102+
uint8 payloadId = encoded.toUint8(index);
103+
104+
// Skip remaining unknown header bytes
105+
index += hdrSize;
106+
107+
// Payload ID of 2 required for batch headerBa
108+
require(payloadId == 2, "invalid payload ID, expected 2 for BatchPriceAttestation");
109+
}
69110

70-
uint16 versionMajor = encoded.toUint16(index);
111+
// Parse the number of attestations
112+
uint16 nAttestations = encoded.toUint16(index);
71113
index += 2;
72-
require(versionMajor == 3, "invalid version major, expected 3");
73114

74-
uint16 versionMinor = encoded.toUint16(index);
115+
// Parse the attestation size
116+
uint16 attestationSize = encoded.toUint16(index);
75117
index += 2;
76-
require(versionMinor >= 0, "invalid version minor, expected 0 or more");
77118

78-
uint16 hdrSize = encoded.toUint16(index);
79-
index += 2;
119+
// Given the message is valid the arithmetic below should not overflow, and
120+
// even if it overflows then the require would fail.
121+
require(encoded.length == (index + (attestationSize * nAttestations)), "invalid BatchPriceAttestation size");
80122

81-
// NOTE(2022-04-19): Currently, only payloadId comes after
82-
// hdrSize. Future extra header fields must be read using a
83-
// separate offset to respect hdrSize, i.e.:
84-
//
85-
// uint hdrIndex = 0;
86-
// bpa.header.payloadId = encoded.toUint8(index + hdrIndex);
87-
// hdrIndex += 1;
88-
//
89-
// bpa.header.someNewField = encoded.toUint32(index + hdrIndex);
90-
// hdrIndex += 4;
91-
//
92-
// // Skip remaining unknown header bytes
93-
// index += bpa.header.hdrSize;
94-
95-
uint8 payloadId = encoded.toUint8(index);
96-
97-
// Skip remaining unknown header bytes
98-
index += hdrSize;
99-
100-
// Payload ID of 2 required for batch headerBa
101-
require(payloadId == 2, "invalid payload ID, expected 2 for BatchPriceAttestation");
102-
}
123+
PythInternalStructs.PriceInfo memory info;
124+
bytes32 priceId;
125+
uint freshPrices = 0;
126+
127+
// Deserialize each attestation
128+
for (uint j=0; j < nAttestations; j++) {
129+
// NOTE: We don't advance the global index immediately.
130+
// attestationIndex is an attestation-local offset used
131+
// for readability and easier debugging.
132+
uint attestationIndex = 0;
103133

104-
// Parse the number of attestations
105-
uint16 nAttestations = encoded.toUint16(index);
106-
index += 2;
134+
// Unused bytes32 product id
135+
attestationIndex += 32;
107136

108-
// Parse the attestation size
109-
uint16 attestationSize = encoded.toUint16(index);
110-
index += 2;
111-
require(encoded.length == (index + (attestationSize * nAttestations)), "invalid BatchPriceAttestation size");
137+
priceId = encoded.toBytes32(index + attestationIndex);
138+
attestationIndex += 32;
112139

113-
PythInternalStructs.PriceInfo memory info;
114-
bytes32 priceId;
115-
uint freshPrices = 0;
116-
117-
// Deserialize each attestation
118-
for (uint j=0; j < nAttestations; j++) {
119-
// NOTE: We don't advance the global index immediately.
120-
// attestationIndex is an attestation-local offset used
121-
// for readability and easier debugging.
122-
uint attestationIndex = 0;
140+
info.price.price = int64(encoded.toUint64(index + attestationIndex));
141+
attestationIndex += 8;
123142

124-
// Unused bytes32 product id
125-
attestationIndex += 32;
143+
info.price.conf = encoded.toUint64(index + attestationIndex);
144+
attestationIndex += 8;
126145

127-
priceId = encoded.toBytes32(index + attestationIndex);
128-
attestationIndex += 32;
146+
info.price.expo = int32(encoded.toUint32(index + attestationIndex));
147+
info.emaPrice.expo = info.price.expo;
148+
attestationIndex += 4;
129149

130-
info.price.price = int64(encoded.toUint64(index + attestationIndex));
131-
attestationIndex += 8;
150+
info.emaPrice.price = int64(encoded.toUint64(index + attestationIndex));
151+
attestationIndex += 8;
132152

133-
info.price.conf = encoded.toUint64(index + attestationIndex);
134-
attestationIndex += 8;
153+
info.emaPrice.conf = encoded.toUint64(index + attestationIndex);
154+
attestationIndex += 8;
135155

136-
info.price.expo = int32(encoded.toUint32(index + attestationIndex));
137-
info.emaPrice.expo = info.price.expo;
138-
attestationIndex += 4;
156+
{
157+
// Status is an enum (encoded as uint8) with the following values:
158+
// 0 = UNKNOWN: The price feed is not currently updating for an unknown reason.
159+
// 1 = TRADING: The price feed is updating as expected.
160+
// 2 = HALTED: The price feed is not currently updating because trading in the product has been halted.
161+
// 3 = AUCTION: The price feed is not currently updating because an auction is setting the price.
162+
uint8 status = encoded.toUint8(index + attestationIndex);
163+
attestationIndex += 1;
139164

140-
info.emaPrice.price = int64(encoded.toUint64(index + attestationIndex));
141-
attestationIndex += 8;
165+
// Unused uint32 numPublishers
166+
attestationIndex += 4;
142167

143-
info.emaPrice.conf = encoded.toUint64(index + attestationIndex);
144-
attestationIndex += 8;
168+
// Unused uint32 numPublishers
169+
attestationIndex += 4;
145170

146-
{
147-
// Status is an enum (encoded as uint8) with the following values:
148-
// 0 = UNKNOWN: The price feed is not currently updating for an unknown reason.
149-
// 1 = TRADING: The price feed is updating as expected.
150-
// 2 = HALTED: The price feed is not currently updating because trading in the product has been halted.
151-
// 3 = AUCTION: The price feed is not currently updating because an auction is setting the price.
152-
uint8 status = encoded.toUint8(index + attestationIndex);
153-
attestationIndex += 1;
154-
155-
// Unused uint32 numPublishers
156-
attestationIndex += 4;
171+
// Unused uint64 attestationTime
172+
attestationIndex += 8;
157173

158-
// Unused uint32 numPublishers
159-
attestationIndex += 4;
174+
info.price.publishTime = encoded.toUint64(index + attestationIndex);
175+
info.emaPrice.publishTime = info.price.publishTime;
176+
attestationIndex += 8;
160177

161-
// Unused uint64 attestationTime
162-
attestationIndex += 8;
178+
if (status == 1) { // status == TRADING
179+
attestationIndex += 24;
180+
} else {
181+
// If status is not trading then the latest available price is
182+
// the previous price info that are passed here.
163183

164-
info.price.publishTime = encoded.toUint64(index + attestationIndex);
165-
info.emaPrice.publishTime = info.price.publishTime;
166-
attestationIndex += 8;
184+
// Previous publish time
185+
info.price.publishTime = encoded.toUint64(index + attestationIndex);
186+
attestationIndex += 8;
167187

168-
if (status == 1) { // status == TRADING
169-
attestationIndex += 24;
170-
} else {
171-
// If status is not trading then the latest available price is
172-
// the previous price info that are passed here.
188+
// Previous price
189+
info.price.price = int64(encoded.toUint64(index + attestationIndex));
190+
attestationIndex += 8;
173191

174-
// Previous publish time
175-
info.price.publishTime = encoded.toUint64(index + attestationIndex);
176-
attestationIndex += 8;
192+
// Previous confidence
193+
info.price.conf = encoded.toUint64(index + attestationIndex);
194+
attestationIndex += 8;
177195

178-
// Previous price
179-
info.price.price = int64(encoded.toUint64(index + attestationIndex));
180-
attestationIndex += 8;
196+
// The EMA is last updated when the aggregate had trading status,
197+
// so, we use previous publish time here too.
198+
info.emaPrice.publishTime = info.price.publishTime;
199+
}
200+
}
181201

182-
// Previous confidence
183-
info.price.conf = encoded.toUint64(index + attestationIndex);
184-
attestationIndex += 8;
185202

186-
// The EMA is last updated when the aggregate had trading status,
187-
// so, we use previous publish time here too.
188-
info.emaPrice.publishTime = info.price.publishTime;
189-
}
190-
}
203+
require(attestationIndex <= attestationSize, "INTERNAL: Consumed more than `attestationSize` bytes");
191204

192-
require(attestationIndex <= attestationSize, "INTERNAL: Consumed more than `attestationSize` bytes");
205+
// Respect specified attestation size for forward-compat
206+
index += attestationSize;
193207

194-
// Respect specified attestation size for forward-compat
195-
index += attestationSize;
208+
// Store the attestation
209+
uint64 latestPublishTime = latestPriceInfoPublishTime(priceId);
196210

197-
// Store the attestation
198-
uint64 latestPublishTime = latestPriceInfoPublishTime(priceId);
211+
bool fresh = false;
212+
if(info.price.publishTime > latestPublishTime) {
213+
freshPrices += 1;
214+
fresh = true;
215+
setLatestPriceInfo(priceId, info);
216+
}
199217

200-
bool fresh = false;
201-
if(info.price.publishTime > latestPublishTime) {
202-
freshPrices += 1;
203-
fresh = true;
204-
setLatestPriceInfo(priceId, info);
218+
emit PriceFeedUpdate(priceId, fresh, vm.emitterChainId, vm.sequence, latestPublishTime,
219+
info.price.publishTime, info.price.price, info.price.conf);
205220
}
206221

207-
emit PriceFeedUpdate(priceId, fresh, vm.emitterChainId, vm.sequence, latestPublishTime,
208-
info.price.publishTime, info.price.price, info.price.conf);
209-
}
210-
211222

212-
emit BatchPriceFeedUpdate(vm.emitterChainId, vm.sequence, nAttestations, freshPrices);
223+
emit BatchPriceFeedUpdate(vm.emitterChainId, vm.sequence, nAttestations, freshPrices);
224+
}
213225
}
214226

215227
function queryPriceFeed(bytes32 id) public view override returns (PythStructs.PriceFeed memory priceFeed){

0 commit comments

Comments
 (0)