Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Remediation of Issues from Filecoin Solidity Audit by Zellic #66

Merged
merged 8 commits into from
Feb 7, 2025
Merged

Remediation of Issues from Filecoin Solidity Audit by Zellic #66

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
8 commits
Select commit Hold shift + click to select a range
d6bc37b
Add validation to prevent uint32 truncation in readUInt32
snissn Jan 11, 2025
9bf8817
Add input validation for BigInt sign byte in deserialization
snissn Jan 11, 2025
1eabac4
Add strict array length check in GetDealDataCommitmentReturn deserial…
snissn Jan 11, 2025
14d60ce
Fix capacity miscalculation in serializeExtendClaimTermsParams
snissn Jan 11, 2025
09f2f20
Fix: Standardize BigInt deserialization with deserializeBytesBigInt a…
snissn Jan 14, 2025
ff0b386
Fix: Revert deserializeBytesBigInt to deserializeBigInt in MarketCBOR…
snissn Jan 14, 2025
acf7e81
fix: Align buffer allocation types in serializeChangeWorkerAddressParams
snissn Jan 14, 2025
7228593
refactor: Replace assert with if + revert using custom errors
snissn Jan 14, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
6 changes: 5 additions & 1 deletion contracts/v0.8/cbor/AccountCbor.sol
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -25,6 +25,7 @@ import "../types/AccountTypes.sol";

import "../utils/CborDecode.sol";
import "../utils/Misc.sol";
import "../utils/Errors.sol";

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

(len, byteIdx) = rawResp.readFixedArray(byteIdx);
assert(len == 2);
if (!(len == 2)) {
revert Errors.InvalidArrayLength(2, len);
}


(ret.signature, byteIdx) = rawResp.readBytes(byteIdx);
(ret.message, byteIdx) = rawResp.readBytes(byteIdx);
Expand Down
3 changes: 3 additions & 0 deletions contracts/v0.8/cbor/BigIntCbor.sol
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -49,6 +49,9 @@ library BigIntCBOR {
return CommonTypes.BigInt(hex"00", false);
}

// Validate the sign byte
require(raw[0] == 0x00 || raw[0] == 0x01, "Invalid sign byte: must be 0x00 or 0x01");

bytes memory val = new bytes(raw.length - 1);
bool neg = false;

Expand Down
14 changes: 11 additions & 3 deletions contracts/v0.8/cbor/DataCapCbor.sol
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -87,7 +87,10 @@ library DataCapCBOR {
bytes memory tmp;

(len, byteIdx) = rawResp.readFixedArray(byteIdx);
assert(len == 3);
if (!(len == 3)) {
revert Errors.InvalidArrayLength(3, len);
}


(tmp, byteIdx) = rawResp.readBytes(byteIdx);
ret.from_balance = tmp.deserializeBigInt();
Expand Down Expand Up @@ -133,7 +136,9 @@ library DataCapCBOR {
bytes memory tmp;

(len, byteIdx) = rawResp.readFixedArray(byteIdx);
assert(len == 4);
if (!(len == 4)) {
revert Errors.InvalidArrayLength(4, len);
}

(tmp, byteIdx) = rawResp.readBytes(byteIdx);
ret.from_balance = tmp.deserializeBigInt();
Expand Down Expand Up @@ -217,7 +222,10 @@ library DataCapCBOR {
bytes memory tmp;

(len, byteIdx) = rawResp.readFixedArray(byteIdx);
assert(len == 2);
if (!(len == 2)) {
revert Errors.InvalidArrayLength(2, len);
}


(tmp, byteIdx) = rawResp.readBytes(byteIdx);
ret.balance = tmp.deserializeBigInt();
Expand Down
6 changes: 5 additions & 1 deletion contracts/v0.8/cbor/FilecoinCbor.sol
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -24,6 +24,7 @@ import "@ensdomains/buffer/contracts/Buffer.sol";

import "../utils/CborDecode.sol";
import "../utils/Misc.sol";
import "../utils/Errors.sol";

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

Expand Down Expand Up @@ -145,7 +146,10 @@ library FilecoinCBOR {
bytes memory tmp;

(len, byteIdx) = rawResp.readFixedArray(byteIdx);
assert(len == 1);
if (!(len == 1)) {
revert Errors.InvalidArrayLength(1, len);
}


(tmp, byteIdx) = rawResp.readBytes(byteIdx);
return tmp.deserializeBigInt();
Expand Down
31 changes: 24 additions & 7 deletions contracts/v0.8/cbor/MarketCbor.sol
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -25,6 +25,7 @@ import "../types/MarketTypes.sol";
import "../types/CommonTypes.sol";

import "../utils/Misc.sol";
import "../utils/Errors.sol";
import "../utils/FilAddresses.sol";
import "../utils/CborDecode.sol";

Expand All @@ -37,6 +38,7 @@ import "./FilecoinCbor.sol";
library MarketCBOR {
using CBOR for CBOR.CBORBuffer;
using CBORDecoder for bytes;

using BigIntCBOR for *;
using FilecoinCBOR for *;

Expand Down Expand Up @@ -68,7 +70,9 @@ library MarketCBOR {
bytes memory tmp;

(len, byteIdx) = rawResp.readFixedArray(byteIdx);
assert(len == 2);
if (!(len == 2)) {
revert Errors.InvalidArrayLength(2, len);
}

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

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

if (len > 0) {
// Ensure the array length is exactly 2 or 0
require(len == 2 || len == 0, "Invalid array length: must be 0 or 2");

if (len == 2) {
(ret.data, byteIdx) = rawResp.readBytes(byteIdx);
(ret.size, byteIdx) = rawResp.readUInt64(byteIdx);
} else {
Expand All @@ -107,7 +114,9 @@ library MarketCBOR {
uint len;

(len, byteIdx) = rawResp.readFixedArray(byteIdx);
assert(len == 2);
if (!(len == 2)) {
revert Errors.InvalidArrayLength(2, len);
}

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

(len, byteIdx) = rawResp.readFixedArray(byteIdx);
assert(len == 2);
if (!(len == 2)) {
revert Errors.InvalidArrayLength(2, len);
}

(ret.activated, byteIdx) = rawResp.readChainEpoch(byteIdx);
(ret.terminated, byteIdx) = rawResp.readChainEpoch(byteIdx);
Expand Down Expand Up @@ -195,7 +206,9 @@ library MarketCBOR {
uint len;

(len, byteIdx) = rawResp.readFixedArray(byteIdx);
assert(len == 2);
if (!(len == 2)) {
revert Errors.InvalidArrayLength(2, len);
}

(len, byteIdx) = rawResp.readFixedArray(byteIdx);
ret.ids = new uint64[](len);
Expand Down Expand Up @@ -226,7 +239,9 @@ library MarketCBOR {
uint len;

(len, byteIdx) = rawResp.readFixedArray(byteIdx);
assert(len == 2);
if (!(len == 2)) {
revert Errors.InvalidArrayLength(2, len);
}

(ret.dealProposal, byteIdx) = rawResp.readBytes(byteIdx);
(ret.dealId, byteIdx) = rawResp.readUInt64(byteIdx);
Expand Down Expand Up @@ -275,7 +290,9 @@ library MarketCBOR {
bytes memory tmp;

(len, byteIdx) = rawResp.readFixedArray(byteIdx);
assert(len == 11);
if (!(len == 11)) {
revert Errors.InvalidArrayLength(11, len);
}

(ret.piece_cid, byteIdx) = rawResp.readCid(byteIdx);
(ret.piece_size, byteIdx) = rawResp.readUInt64(byteIdx);
Expand Down
52 changes: 37 additions & 15 deletions contracts/v0.8/cbor/MinerCbor.sol
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -23,18 +23,21 @@ import "solidity-cborutils/contracts/CBOR.sol";

import "./BigIntCbor.sol";
import "./FilecoinCbor.sol";
import "./BytesCbor.sol";

import "../types/MinerTypes.sol";
import "../types/CommonTypes.sol";

import "../utils/CborDecode.sol";
import "../utils/Misc.sol";
import "../utils/Errors.sol";

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

Expand Down Expand Up @@ -67,7 +70,9 @@ library MinerCBOR {
uint len;

(len, byteIdx) = rawResp.readFixedArray(byteIdx);
assert(len == 2);
if (!(len == 2)) {
revert Errors.InvalidArrayLength(2, len);
}

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

Expand All @@ -89,26 +94,32 @@ library MinerCBOR {
uint len;

(len, byteIdx) = rawResp.readFixedArray(byteIdx);
assert(len == 2);
if (!(len == 2)) {
revert Errors.InvalidArrayLength(2, len);
}

(len, byteIdx) = rawResp.readFixedArray(byteIdx);
assert(len == 2);
if (!(len == 2)) {
revert Errors.InvalidArrayLength(2, len);
}

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

(len, byteIdx) = rawResp.readFixedArray(byteIdx);
assert(len == 3);
if (!(len == 3)) {
revert Errors.InvalidArrayLength(3, len);
}

(tmp, byteIdx) = rawResp.readBytes(byteIdx);
if (tmp.length > 0) {
ret.active.term.quota = tmp.deserializeBigInt();
ret.active.term.quota = tmp.deserializeBytesBigInt();
} else {
ret.active.term.quota = CommonTypes.BigInt(new bytes(0), false);
}

(tmp, byteIdx) = rawResp.readBytes(byteIdx);
if (tmp.length > 0) {
ret.active.term.used_quota = tmp.deserializeBigInt();
ret.active.term.used_quota = tmp.deserializeBytesBigInt();
} else {
ret.active.term.used_quota = CommonTypes.BigInt(new bytes(0), false);
}
Expand All @@ -117,13 +128,15 @@ library MinerCBOR {

if (!rawResp.isNullNext(byteIdx)) {
(len, byteIdx) = rawResp.readFixedArray(byteIdx);
assert(len == 5);
if (!(len == 5)) {
revert Errors.InvalidArrayLength(5, len);
}

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

(tmp, byteIdx) = rawResp.readBytes(byteIdx);
if (tmp.length > 0) {
ret.proposed.new_quota = tmp.deserializeBigInt();
ret.proposed.new_quota = tmp.deserializeBytesBigInt();
} else {
ret.proposed.new_quota = CommonTypes.BigInt(new bytes(0), false);
}
Expand All @@ -149,7 +162,9 @@ library MinerCBOR {
uint leni;

(len, byteIdx) = rawResp.readFixedArray(byteIdx);
assert(len == 1);
if (len != 1) {
revert Errors.InvalidArrayLength(1, len);
}

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

amount = tmp.deserializeBigInt();
amount = tmp.deserializeBytesBigInt();
vesting_funds[i] = MinerTypes.VestingFunds(epoch, amount);
}
}
Expand All @@ -171,20 +186,25 @@ library MinerCBOR {
/// @return cbor serialized data as bytes
function serializeChangeWorkerAddressParams(MinerTypes.ChangeWorkerAddressParams memory params) internal pure returns (bytes memory) {
uint256 capacity = 0;
uint64 addressCount = uint64(params.new_control_addresses.length);

// Safety check to prevent silent truncation
require(params.new_control_addresses.length == addressCount, "Address count exceeds uint64 limit");

capacity += Misc.getPrefixSize(2);
capacity += Misc.getBytesSize(params.new_worker.data);
capacity += Misc.getPrefixSize(uint256(params.new_control_addresses.length));
for (uint64 i = 0; i < params.new_control_addresses.length; i++) {
capacity += Misc.getPrefixSize(addressCount);

for (uint64 i = 0; i < addressCount; i++) {
capacity += Misc.getBytesSize(params.new_control_addresses[i].data);
}
CBOR.CBORBuffer memory buf = CBOR.create(capacity);

buf.startFixedArray(2);
buf.writeBytes(params.new_worker.data);
buf.startFixedArray(uint64(params.new_control_addresses.length));
buf.startFixedArray(addressCount);

for (uint64 i = 0; i < params.new_control_addresses.length; i++) {
for (uint64 i = 0; i < addressCount; i++) {
buf.writeBytes(params.new_control_addresses[i].data);
}

Expand Down Expand Up @@ -222,7 +242,9 @@ library MinerCBOR {
uint len;

(len, byteIdx) = rawResp.readFixedArray(byteIdx);
assert(len == 1);
if (len != 1) {
revert Errors.InvalidArrayLength(1, len);
}

(len, byteIdx) = rawResp.readFixedArray(byteIdx);
multi_addrs = new CommonTypes.FilAddress[](len);
Expand Down
13 changes: 10 additions & 3 deletions contracts/v0.8/cbor/PowerCbor.sol
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -25,13 +25,16 @@ import "../types/CommonTypes.sol";
import "../types/PowerTypes.sol";
import "../utils/CborDecode.sol";
import "../utils/Misc.sol";
import "../utils/Errors.sol";
import "./BigIntCbor.sol";
import "./BytesCbor.sol";

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

Expand Down Expand Up @@ -74,7 +77,9 @@ library PowerCBOR {
uint len;

(len, byteIdx) = rawResp.readFixedArray(byteIdx);
assert(len == 2);
if (!(len == 2)) {
revert Errors.InvalidArrayLength(2, len);
}

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

(len, byteIdx) = rawResp.readFixedArray(byteIdx);
assert(len == 2);
if (!(len == 2)) {
revert Errors.InvalidArrayLength(2, len);
}

bytes memory tmp;
(tmp, byteIdx) = rawResp.readBytes(byteIdx);
if (tmp.length > 0) {
ret.raw_byte_power = tmp.deserializeBigInt();
ret.raw_byte_power = tmp.deserializeBytesBigInt();
} else {
ret.raw_byte_power = CommonTypes.BigInt(new bytes(0), false);
}
Expand Down
Loading
Loading