Skip to content

Commit c72ec4b

Browse files
trruckerflingtrruckerfling
authored
Merge pull request #66 from filecoin-project/zellic-remediation
Remediation of Issues from Filecoin Solidity Audit by Zellic
2 parents f655709 + 7228593 commit c72ec4b

10 files changed

+160
-47
lines changed

‎contracts/v0.8/cbor/AccountCbor.sol

+5-1
Original file line numberDiff line numberDiff line change
@@ -25,6 +25,7 @@ import "../types/AccountTypes.sol";
2525

2626
import "../utils/CborDecode.sol";
2727
import "../utils/Misc.sol";
28+
import "../utils/Errors.sol";
2829

2930
/// @title This library is a set of functions meant to handle CBOR parameters serialization and return values deserialization for Account actor exported methods.
3031
/// @author Zondax AG
@@ -59,7 +60,10 @@ library AccountCBOR {
5960
uint len;
6061

6162
(len, byteIdx) = rawResp.readFixedArray(byteIdx);
62-
assert(len == 2);
63+
if (!(len == 2)) {
64+
revert Errors.InvalidArrayLength(2, len);
65+
}
66+
6367

6468
(ret.signature, byteIdx) = rawResp.readBytes(byteIdx);
6569
(ret.message, byteIdx) = rawResp.readBytes(byteIdx);

‎contracts/v0.8/cbor/BigIntCbor.sol

+3
Original file line numberDiff line numberDiff line change
@@ -49,6 +49,9 @@ library BigIntCBOR {
4949
return CommonTypes.BigInt(hex"00", false);
5050
}
5151

52+
// Validate the sign byte
53+
require(raw[0] == 0x00 || raw[0] == 0x01, "Invalid sign byte: must be 0x00 or 0x01");
54+
5255
bytes memory val = new bytes(raw.length - 1);
5356
bool neg = false;
5457

‎contracts/v0.8/cbor/DataCapCbor.sol

+11-3
Original file line numberDiff line numberDiff line change
@@ -87,7 +87,10 @@ library DataCapCBOR {
8787
bytes memory tmp;
8888

8989
(len, byteIdx) = rawResp.readFixedArray(byteIdx);
90-
assert(len == 3);
90+
if (!(len == 3)) {
91+
revert Errors.InvalidArrayLength(3, len);
92+
}
93+
9194

9295
(tmp, byteIdx) = rawResp.readBytes(byteIdx);
9396
ret.from_balance = tmp.deserializeBigInt();
@@ -133,7 +136,9 @@ library DataCapCBOR {
133136
bytes memory tmp;
134137

135138
(len, byteIdx) = rawResp.readFixedArray(byteIdx);
136-
assert(len == 4);
139+
if (!(len == 4)) {
140+
revert Errors.InvalidArrayLength(4, len);
141+
}
137142

138143
(tmp, byteIdx) = rawResp.readBytes(byteIdx);
139144
ret.from_balance = tmp.deserializeBigInt();
@@ -217,7 +222,10 @@ library DataCapCBOR {
217222
bytes memory tmp;
218223

219224
(len, byteIdx) = rawResp.readFixedArray(byteIdx);
220-
assert(len == 2);
225+
if (!(len == 2)) {
226+
revert Errors.InvalidArrayLength(2, len);
227+
}
228+
221229

222230
(tmp, byteIdx) = rawResp.readBytes(byteIdx);
223231
ret.balance = tmp.deserializeBigInt();

‎contracts/v0.8/cbor/FilecoinCbor.sol

+5-1
Original file line numberDiff line numberDiff line change
@@ -24,6 +24,7 @@ import "@ensdomains/buffer/contracts/Buffer.sol";
2424

2525
import "../utils/CborDecode.sol";
2626
import "../utils/Misc.sol";
27+
import "../utils/Errors.sol";
2728

2829
import "../types/CommonTypes.sol";
2930

@@ -145,7 +146,10 @@ library FilecoinCBOR {
145146
bytes memory tmp;
146147

147148
(len, byteIdx) = rawResp.readFixedArray(byteIdx);
148-
assert(len == 1);
149+
if (!(len == 1)) {
150+
revert Errors.InvalidArrayLength(1, len);
151+
}
152+
149153

150154
(tmp, byteIdx) = rawResp.readBytes(byteIdx);
151155
return tmp.deserializeBigInt();

‎contracts/v0.8/cbor/MarketCbor.sol

+24-7
Original file line numberDiff line numberDiff line change
@@ -25,6 +25,7 @@ import "../types/MarketTypes.sol";
2525
import "../types/CommonTypes.sol";
2626

2727
import "../utils/Misc.sol";
28+
import "../utils/Errors.sol";
2829
import "../utils/FilAddresses.sol";
2930
import "../utils/CborDecode.sol";
3031

@@ -37,6 +38,7 @@ import "./FilecoinCbor.sol";
3738
library MarketCBOR {
3839
using CBOR for CBOR.CBORBuffer;
3940
using CBORDecoder for bytes;
41+
4042
using BigIntCBOR for *;
4143
using FilecoinCBOR for *;
4244

@@ -68,7 +70,9 @@ library MarketCBOR {
6870
bytes memory tmp;
6971

7072
(len, byteIdx) = rawResp.readFixedArray(byteIdx);
71-
assert(len == 2);
73+
if (!(len == 2)) {
74+
revert Errors.InvalidArrayLength(2, len);
75+
}
7276

7377
(tmp, byteIdx) = rawResp.readBytes(byteIdx);
7478
ret.balance = tmp.deserializeBigInt();
@@ -88,7 +92,10 @@ library MarketCBOR {
8892

8993
(len, byteIdx) = rawResp.readFixedArray(byteIdx);
9094

91-
if (len > 0) {
95+
// Ensure the array length is exactly 2 or 0
96+
require(len == 2 || len == 0, "Invalid array length: must be 0 or 2");
97+
98+
if (len == 2) {
9299
(ret.data, byteIdx) = rawResp.readBytes(byteIdx);
93100
(ret.size, byteIdx) = rawResp.readUInt64(byteIdx);
94101
} else {
@@ -107,7 +114,9 @@ library MarketCBOR {
107114
uint len;
108115

109116
(len, byteIdx) = rawResp.readFixedArray(byteIdx);
110-
assert(len == 2);
117+
if (!(len == 2)) {
118+
revert Errors.InvalidArrayLength(2, len);
119+
}
111120

112121
(ret.start, byteIdx) = rawResp.readChainEpoch(byteIdx);
113122
(ret.duration, byteIdx) = rawResp.readChainEpoch(byteIdx);
@@ -123,7 +132,9 @@ library MarketCBOR {
123132
uint len;
124133

125134
(len, byteIdx) = rawResp.readFixedArray(byteIdx);
126-
assert(len == 2);
135+
if (!(len == 2)) {
136+
revert Errors.InvalidArrayLength(2, len);
137+
}
127138

128139
(ret.activated, byteIdx) = rawResp.readChainEpoch(byteIdx);
129140
(ret.terminated, byteIdx) = rawResp.readChainEpoch(byteIdx);
@@ -195,7 +206,9 @@ library MarketCBOR {
195206
uint len;
196207

197208
(len, byteIdx) = rawResp.readFixedArray(byteIdx);
198-
assert(len == 2);
209+
if (!(len == 2)) {
210+
revert Errors.InvalidArrayLength(2, len);
211+
}
199212

200213
(len, byteIdx) = rawResp.readFixedArray(byteIdx);
201214
ret.ids = new uint64[](len);
@@ -226,7 +239,9 @@ library MarketCBOR {
226239
uint len;
227240

228241
(len, byteIdx) = rawResp.readFixedArray(byteIdx);
229-
assert(len == 2);
242+
if (!(len == 2)) {
243+
revert Errors.InvalidArrayLength(2, len);
244+
}
230245

231246
(ret.dealProposal, byteIdx) = rawResp.readBytes(byteIdx);
232247
(ret.dealId, byteIdx) = rawResp.readUInt64(byteIdx);
@@ -275,7 +290,9 @@ library MarketCBOR {
275290
bytes memory tmp;
276291

277292
(len, byteIdx) = rawResp.readFixedArray(byteIdx);
278-
assert(len == 11);
293+
if (!(len == 11)) {
294+
revert Errors.InvalidArrayLength(11, len);
295+
}
279296

280297
(ret.piece_cid, byteIdx) = rawResp.readCid(byteIdx);
281298
(ret.piece_size, byteIdx) = rawResp.readUInt64(byteIdx);

‎contracts/v0.8/cbor/MinerCbor.sol

+37-15
Original file line numberDiff line numberDiff line change
@@ -23,18 +23,21 @@ import "solidity-cborutils/contracts/CBOR.sol";
2323

2424
import "./BigIntCbor.sol";
2525
import "./FilecoinCbor.sol";
26+
import "./BytesCbor.sol";
2627

2728
import "../types/MinerTypes.sol";
2829
import "../types/CommonTypes.sol";
2930

3031
import "../utils/CborDecode.sol";
3132
import "../utils/Misc.sol";
33+
import "../utils/Errors.sol";
3234

3335
/// @title This library is a set of functions meant to handle CBOR parameters serialization and return values deserialization for Miner actor exported methods.
3436
/// @author Zondax AG
3537
library MinerCBOR {
3638
using CBOR for CBOR.CBORBuffer;
3739
using CBORDecoder for bytes;
40+
using BytesCBOR for bytes;
3841
using BigIntCBOR for *;
3942
using FilecoinCBOR for *;
4043

@@ -67,7 +70,9 @@ library MinerCBOR {
6770
uint len;
6871

6972
(len, byteIdx) = rawResp.readFixedArray(byteIdx);
70-
assert(len == 2);
73+
if (!(len == 2)) {
74+
revert Errors.InvalidArrayLength(2, len);
75+
}
7176

7277
(ret.owner.data, byteIdx) = rawResp.readBytes(byteIdx);
7378

@@ -89,26 +94,32 @@ library MinerCBOR {
8994
uint len;
9095

9196
(len, byteIdx) = rawResp.readFixedArray(byteIdx);
92-
assert(len == 2);
97+
if (!(len == 2)) {
98+
revert Errors.InvalidArrayLength(2, len);
99+
}
93100

94101
(len, byteIdx) = rawResp.readFixedArray(byteIdx);
95-
assert(len == 2);
102+
if (!(len == 2)) {
103+
revert Errors.InvalidArrayLength(2, len);
104+
}
96105

97106
(ret.active.beneficiary.data, byteIdx) = rawResp.readBytes(byteIdx);
98107

99108
(len, byteIdx) = rawResp.readFixedArray(byteIdx);
100-
assert(len == 3);
109+
if (!(len == 3)) {
110+
revert Errors.InvalidArrayLength(3, len);
111+
}
101112

102113
(tmp, byteIdx) = rawResp.readBytes(byteIdx);
103114
if (tmp.length > 0) {
104-
ret.active.term.quota = tmp.deserializeBigInt();
115+
ret.active.term.quota = tmp.deserializeBytesBigInt();
105116
} else {
106117
ret.active.term.quota = CommonTypes.BigInt(new bytes(0), false);
107118
}
108119

109120
(tmp, byteIdx) = rawResp.readBytes(byteIdx);
110121
if (tmp.length > 0) {
111-
ret.active.term.used_quota = tmp.deserializeBigInt();
122+
ret.active.term.used_quota = tmp.deserializeBytesBigInt();
112123
} else {
113124
ret.active.term.used_quota = CommonTypes.BigInt(new bytes(0), false);
114125
}
@@ -117,13 +128,15 @@ library MinerCBOR {
117128

118129
if (!rawResp.isNullNext(byteIdx)) {
119130
(len, byteIdx) = rawResp.readFixedArray(byteIdx);
120-
assert(len == 5);
131+
if (!(len == 5)) {
132+
revert Errors.InvalidArrayLength(5, len);
133+
}
121134

122135
(ret.proposed.new_beneficiary.data, byteIdx) = rawResp.readBytes(byteIdx);
123136

124137
(tmp, byteIdx) = rawResp.readBytes(byteIdx);
125138
if (tmp.length > 0) {
126-
ret.proposed.new_quota = tmp.deserializeBigInt();
139+
ret.proposed.new_quota = tmp.deserializeBytesBigInt();
127140
} else {
128141
ret.proposed.new_quota = CommonTypes.BigInt(new bytes(0), false);
129142
}
@@ -149,7 +162,9 @@ library MinerCBOR {
149162
uint leni;
150163

151164
(len, byteIdx) = rawResp.readFixedArray(byteIdx);
152-
assert(len == 1);
165+
if (len != 1) {
166+
revert Errors.InvalidArrayLength(1, len);
167+
}
153168

154169
(len, byteIdx) = rawResp.readFixedArray(byteIdx);
155170
vesting_funds = new MinerTypes.VestingFunds[](len);
@@ -161,7 +176,7 @@ library MinerCBOR {
161176
(epoch, byteIdx) = rawResp.readChainEpoch(byteIdx);
162177
(tmp, byteIdx) = rawResp.readBytes(byteIdx);
163178

164-
amount = tmp.deserializeBigInt();
179+
amount = tmp.deserializeBytesBigInt();
165180
vesting_funds[i] = MinerTypes.VestingFunds(epoch, amount);
166181
}
167182
}
@@ -171,20 +186,25 @@ library MinerCBOR {
171186
/// @return cbor serialized data as bytes
172187
function serializeChangeWorkerAddressParams(MinerTypes.ChangeWorkerAddressParams memory params) internal pure returns (bytes memory) {
173188
uint256 capacity = 0;
189+
uint64 addressCount = uint64(params.new_control_addresses.length);
190+
191+
// Safety check to prevent silent truncation
192+
require(params.new_control_addresses.length == addressCount, "Address count exceeds uint64 limit");
174193

175194
capacity += Misc.getPrefixSize(2);
176195
capacity += Misc.getBytesSize(params.new_worker.data);
177-
capacity += Misc.getPrefixSize(uint256(params.new_control_addresses.length));
178-
for (uint64 i = 0; i < params.new_control_addresses.length; i++) {
196+
capacity += Misc.getPrefixSize(addressCount);
197+
198+
for (uint64 i = 0; i < addressCount; i++) {
179199
capacity += Misc.getBytesSize(params.new_control_addresses[i].data);
180200
}
181201
CBOR.CBORBuffer memory buf = CBOR.create(capacity);
182202

183203
buf.startFixedArray(2);
184204
buf.writeBytes(params.new_worker.data);
185-
buf.startFixedArray(uint64(params.new_control_addresses.length));
205+
buf.startFixedArray(addressCount);
186206

187-
for (uint64 i = 0; i < params.new_control_addresses.length; i++) {
207+
for (uint64 i = 0; i < addressCount; i++) {
188208
buf.writeBytes(params.new_control_addresses[i].data);
189209
}
190210

@@ -222,7 +242,9 @@ library MinerCBOR {
222242
uint len;
223243

224244
(len, byteIdx) = rawResp.readFixedArray(byteIdx);
225-
assert(len == 1);
245+
if (len != 1) {
246+
revert Errors.InvalidArrayLength(1, len);
247+
}
226248

227249
(len, byteIdx) = rawResp.readFixedArray(byteIdx);
228250
multi_addrs = new CommonTypes.FilAddress[](len);

‎contracts/v0.8/cbor/PowerCbor.sol

+10-3
Original file line numberDiff line numberDiff line change
@@ -25,13 +25,16 @@ import "../types/CommonTypes.sol";
2525
import "../types/PowerTypes.sol";
2626
import "../utils/CborDecode.sol";
2727
import "../utils/Misc.sol";
28+
import "../utils/Errors.sol";
2829
import "./BigIntCbor.sol";
30+
import "./BytesCbor.sol";
2931

3032
/// @title This library is a set of functions meant to handle CBOR parameters serialization and return values deserialization for Power actor exported methods.
3133
/// @author Zondax AG
3234
library PowerCBOR {
3335
using CBOR for CBOR.CBORBuffer;
3436
using CBORDecoder for bytes;
37+
using BytesCBOR for bytes;
3538
using BigIntCBOR for CommonTypes.BigInt;
3639
using BigIntCBOR for bytes;
3740

@@ -74,7 +77,9 @@ library PowerCBOR {
7477
uint len;
7578

7679
(len, byteIdx) = rawResp.readFixedArray(byteIdx);
77-
assert(len == 2);
80+
if (!(len == 2)) {
81+
revert Errors.InvalidArrayLength(2, len);
82+
}
7883

7984
(ret.id_address.data, byteIdx) = rawResp.readBytes(byteIdx);
8085
(ret.robust_address.data, byteIdx) = rawResp.readBytes(byteIdx);
@@ -90,12 +95,14 @@ library PowerCBOR {
9095
uint len;
9196

9297
(len, byteIdx) = rawResp.readFixedArray(byteIdx);
93-
assert(len == 2);
98+
if (!(len == 2)) {
99+
revert Errors.InvalidArrayLength(2, len);
100+
}
94101

95102
bytes memory tmp;
96103
(tmp, byteIdx) = rawResp.readBytes(byteIdx);
97104
if (tmp.length > 0) {
98-
ret.raw_byte_power = tmp.deserializeBigInt();
105+
ret.raw_byte_power = tmp.deserializeBytesBigInt();
99106
} else {
100107
ret.raw_byte_power = CommonTypes.BigInt(new bytes(0), false);
101108
}

0 commit comments

Comments
 (0)