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

Jump to bottom

Fix UpdateMember encoding & decoding #32

Merged
merged 11 commits into from
Jan 30, 2023
Merged

Fix UpdateMember encoding & decoding #32

Show file tree
Hide file tree
Changes from 8 commits
Commits
Show all changes
11 commits
Select commit Hold shift + click to select a range
7e465f8
wip: Work on fix add member
NunoAlexandre Jan 26, 2023
30d5db7
Fix and test tmp parseUpdateMemberDebug
NunoAlexandre Jan 30, 2023
3fd29fd
wip: Migrate validUntil from uint256 to uint64
NunoAlexandre Jan 30, 2023
db2f5c3
Fix all UpdateMember tests
NunoAlexandre Jan 30, 2023
4f55b21
Clean commented tests
NunoAlexandre Jan 30, 2023
4930ce2
Add testUpdateMemberEncoding
NunoAlexandre Jan 30, 2023
3f80815
Drop testPlayground
NunoAlexandre Jan 30, 2023
3c7be17
Clean testUpdateMemberEquivalence
NunoAlexandre Jan 30, 2023
6c7829f
fixup
NunoAlexandre Jan 30, 2023
c4fb963
Leave comment
NunoAlexandre Jan 30, 2023
b8ab0c7
Merge remote-tracking branch 'origin/add-withdaw-deposit' into fix/ad…
NunoAlexandre Jan 30, 2023
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
2 changes: 1 addition & 1 deletion src/Connector.sol
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -141,7 +141,7 @@ contract CentrifugeConnector {
uint64 poolId,
bytes16 trancheId,
address user,
uint256 validUntil
uint64 validUntil
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This was wrong and often validUntil was named amount, probably from a copy-paste from transfer I guess

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good find!

) public onlyRouter {
Tranche storage tranche = tranches[poolId][trancheId];
require(tranche.latestPrice > 0, "CentrifugeConnector/invalid-pool-or-tranche");
Expand Down
46 changes: 38 additions & 8 deletions src/Messages.sol
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -50,7 +50,7 @@ library ConnectorMessages {
function formatAddTranche(uint64 poolId, bytes16 trancheId, string memory tokenName, string memory tokenSymbol) internal pure returns (bytes memory) {
// TODO(nuno): Now, we encode `tokenName` as a 128-bytearray by first encoding `tokenName`
// to bytes32 and then we encode three empty bytes32's, which sum up to a total of 128 bytes.
// Add support to actually encode `tokennName` fully as a 128 bytes string.
// Add support to actually encode `tokenName` fully as a 128 bytes string.
return abi.encodePacked(uint8(Call.AddTranche), poolId, trancheId, stringToBytes32(tokenName), bytes32(""), bytes32(""), bytes32(""), stringToBytes32(tokenSymbol));
}

Expand Down Expand Up @@ -96,25 +96,55 @@ library ConnectorMessages {
* 0: call type (uint8 = 1 byte)
* 1-8: poolId (uint64 = 8 bytes)
* 9-25: trancheId (16 bytes)
* 26-46: user (Ethereum address, 20 bytes)
* 47-78: validUntil (uint256 = 32 bytes)
* 25-45: user (Ethereum address, 20 bytes - Skip 12 bytes from 32-byte addresses)
* 57-65: validUntil (uint64 = 8 bytes)
*
* TODO: use bytes32 for user (for non-EVM compatibility)
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@offerijns I started working on this but we would need to also update Transfer and the storage items storing address now. I prefer to do that in a follow up PR unless if you think we shouldn't do this now?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think that's totally fine in a separate PR!

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yah separating issues into separate PRs is a good idea. I'll note here so I don't forget though, it looks like we're double-reading a byte here. Tranche ID is 9-25, while user is 25-45.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@AStox I first thought this as well but it doesn't seem to be the case:

  poolId = uint64(_msg.indexUint(1, 8));
  trancheId = bytes16(_msg.index(9, 16));
  user = address(bytes20(_msg.index(25, 20)));
  validUntil = uint64(_msg.indexUint(57, 8));

So, poolId would be 1 + 8 = 9, and then we would be reading trancheId from byte at index 9, which would be another example of double-reading.

The docs were wrong indeed tho, as it's 9-24. Thanks for the catch!

*/
function formatUpdateMember(uint64 poolId, bytes16 trancheId, address user, uint256 validUntil) internal pure returns (bytes memory) {
return abi.encodePacked(uint8(Call.UpdateMember), poolId, trancheId, user, validUntil);
function formatUpdateMember(uint64 poolId, bytes16 trancheId, address user, uint64 validUntil) internal pure returns (bytes memory) {
return abi.encodePacked(uint8(Call.UpdateMember), poolId, trancheId, user, bytes(hex"000000000000000000000000"), validUntil);
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Since the decoding parses the first 20 bytes of user and skips the following 12, here we need to append 12 padding zeros to make it right.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can you add a comment for this?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@offerijns done 👍

}

function isUpdateMember(bytes29 _msg) internal pure returns (bool) {
return messageType(_msg) == Call.UpdateMember;
}

function parseUpdateMember(bytes29 _msg) internal pure returns (uint64 poolId, bytes16 trancheId, address user, uint256 validUntil) {

function parseUpdateMember(bytes29 _msg) internal pure returns (uint64 poolId, bytes16 trancheId, address user, uint64 validUntil) {
poolId = uint64(_msg.indexUint(1, 8));
trancheId = bytes16(_msg.index(9, 16));
user = address(bytes20(_msg.index(25, 20)));
// TODO: skip 12 padded zeroes from address
validUntil = uint256(_msg.index(45, 32));
validUntil = uint64(_msg.indexUint(57, 8));
}

// Convert an hexadecimal character to their value
function fromHexChar(uint8 c) internal pure returns (uint8) {
if (bytes1(c) >= bytes1("0") && bytes1(c) <= bytes1("9")) {
return c - uint8(bytes1("0"));
}
if (bytes1(c) >= bytes1("a") && bytes1(c) <= bytes1("f")) {
return 10 + c - uint8(bytes1("a"));
}
if (bytes1(c) >= bytes1("A") && bytes1(c) <= bytes1("F")) {
return 10 + c - uint8(bytes1("A"));
}
revert("Failed to encode hex char");
}

// Convert an hexadecimal string to raw bytes
function fromHex(string memory s) internal pure returns (bytes memory) {
bytes memory ss = bytes(s);
require(ss.length % 2 == 0); // length must be even
bytes memory r = new bytes(ss.length / 2);

for (uint256 i = 0; i < ss.length / 2; ++i) {
r[i] = bytes1(
fromHexChar(uint8(ss[2 * i])) *
16 +
fromHexChar(uint8(ss[2 * i + 1]))
);
}
return r;
}

/**
Expand Down
6 changes: 3 additions & 3 deletions src/routers/xcm/Router.sol
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -8,7 +8,7 @@ import {ConnectorMessages} from "../../Messages.sol";
interface ConnectorLike {
function addPool(uint64 poolId) external;
function addTranche(uint64 poolId, bytes16 trancheId, string memory tokenName, string memory tokenSymbol) external;
function updateMember(uint64 poolId, bytes16 trancheId, address user, uint256 validUntil) external;
function updateMember(uint64 poolId, bytes16 trancheId, address user, uint64 validUntil) external;
function updateTokenPrice(uint64 poolId, bytes16 trancheId, uint256 price) external;
function handleTransfer(uint64 poolId, bytes16 trancheId, address user, uint256 amount) external;
}
Expand Down Expand Up @@ -50,7 +50,7 @@ contract ConnectorXCMRouter {
(uint64 poolId, bytes16 trancheId, string memory tokenName, string memory tokenSymbol) = ConnectorMessages.parseAddTranche(_msg);
connector.addTranche(poolId, trancheId, tokenName, tokenSymbol);
} else if (ConnectorMessages.isUpdateMember(_msg) == true) {
(uint64 poolId, bytes16 trancheId, address user, uint256 validUntil) = ConnectorMessages.parseUpdateMember(_msg);
(uint64 poolId, bytes16 trancheId, address user, uint64 validUntil) = ConnectorMessages.parseUpdateMember(_msg);
connector.updateMember(poolId, trancheId, user, validUntil);
} else if (ConnectorMessages.isUpdateTokenPrice(_msg) == true) {
(uint64 poolId, bytes16 trancheId, uint256 price) = ConnectorMessages.parseUpdateTokenPrice(_msg);
Expand All @@ -67,7 +67,7 @@ contract ConnectorXCMRouter {
// TODO: implement
}

function bytes32ToString(bytes32 _bytes32) internal returns (string memory) {
function bytes32ToString(bytes32 _bytes32) internal pure returns (string memory) {
uint8 i = 0;
while (i < 32 && _bytes32[i] != 0) {
i++;
Expand Down
16 changes: 7 additions & 9 deletions test/Connector.t.sol
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -92,9 +92,8 @@ contract ConnectorTest is Test {
homeConnector.addTranche(poolId, trancheId, tokenName, tokenSymbol);
}

function testUpdatingMemberWorks(uint64 poolId, bytes16 trancheId, address user, uint128 fuzzed_uint128) public {
vm.assume(fuzzed_uint128 > 0);
uint256 validUntil = safeAdd(fuzzed_uint128, safeAdd(block.timestamp, minimumDelay));
function testUpdatingMemberWorks(uint64 poolId, bytes16 trancheId, address user, uint64 validUntil) public {
vm.assume(validUntil >= safeAdd(block.timestamp, new Memberlist().minimumDelay()));
vm.assume(user != address(0));

homeConnector.addPool(poolId);
Expand All @@ -110,7 +109,7 @@ contract ConnectorTest is Test {
assertEq(memberlist.members(user), validUntil);
}

function testUpdatingMemberBeforeMinimumDelayFails(uint64 poolId, bytes16 trancheId, address user, uint256 validUntil) public {
function testUpdatingMemberBeforeMinimumDelayFails(uint64 poolId, bytes16 trancheId, address user, uint64 validUntil) public {
vm.assume(validUntil <= safeAdd(block.timestamp, new Memberlist().minimumDelay()));
vm.assume(user != address(0));

Expand All @@ -121,24 +120,23 @@ contract ConnectorTest is Test {
homeConnector.updateMember(poolId, trancheId, user, validUntil);
}

function testUpdatingMemberAsNonRouterFails(uint64 poolId, bytes16 trancheId, address user, uint128 fuzzed_uint128) public {
vm.assume(fuzzed_uint128 > 0);
uint256 validUntil = safeAdd(fuzzed_uint128, safeAdd(block.timestamp, new Memberlist().minimumDelay()));
function testUpdatingMemberAsNonRouterFails(uint64 poolId, bytes16 trancheId, address user, uint64 validUntil) public {
vm.assume(validUntil <= safeAdd(block.timestamp, new Memberlist().minimumDelay()));
vm.assume(user != address(0));

vm.expectRevert(bytes("CentrifugeConnector/not-the-router"));
bridgedConnector.updateMember(poolId, trancheId, user, validUntil);
}

function testUpdatingMemberForNonExistentPoolFails(uint64 poolId, bytes16 trancheId, address user, uint256 validUntil) public {
function testUpdatingMemberForNonExistentPoolFails(uint64 poolId, bytes16 trancheId, address user, uint64 validUntil) public {
vm.assume(validUntil > block.timestamp);
bridgedConnector.file("router", address(this));
vm.expectRevert(bytes("CentrifugeConnector/invalid-pool-or-tranche"));
bridgedConnector.updateMember(poolId, trancheId, user, validUntil);
}


function testUpdatingMemberForNonExistentTrancheFails(uint64 poolId, bytes16 trancheId, address user, uint256 validUntil) public {
function testUpdatingMemberForNonExistentTrancheFails(uint64 poolId, bytes16 trancheId, address user, uint64 validUntil) public {
vm.assume(validUntil > block.timestamp);
bridgedConnector.file("router", address(this));
bridgedConnector.addPool(poolId);
Expand Down
26 changes: 13 additions & 13 deletions test/Messages.t.sol
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -91,41 +91,41 @@ contract MessagesTest is Test {

function testUpdateMemberEncoding() public {
assertEq(
ConnectorMessages.formatUpdateMember(5, toBytes16(fromHex("010000000000000003")), 0x225ef95fa90f4F7938A5b34234d14768cB4263dd, 1657870537),
fromHex("04000000000000000500000000000000000000000000000009225ef95fa90f4f7938a5b34234d14768cb4263dd0000000000000000000000000000000000000000000000000000000062d118c9")
);
ConnectorMessages.formatUpdateMember(2, bytes16(hex"811acd5b3f17c06841c7e41e9e04cb1b"), 0x1231231231231231231231231231231231231231, 1706260138),
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

An interesting and frustration finding was that toBytes16(fromHex("811acd5b3f17c06841c7e41e9e04cb1b")) was retuning me an empty bytes16 🤷‍♂️

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Odd... does fromHex() expect a prepended 0x maybe?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Doesn't really look like it based on how we use it elsewhere actually.

hex"040000000000000002811acd5b3f17c06841c7e41e9e04cb1b12312312312312312312312312312312312312310000000000000000000000000000000065b376aa"
);
}

function testUpdateMemberDecoding() public {
(uint64 decodedPoolId, bytes16 decodedTrancheId, address decodedUser, uint256 decodedValidUntil) = ConnectorMessages.parseUpdateMember(fromHex("04000000000000000500000000000000000000000000000009225ef95fa90f4f7938a5b34234d14768cb4263dd0000000000000000000000000000000000000000000000000000000062d118c9").ref(0));
assertEq(uint(decodedPoolId), uint(5));
assertEq(decodedTrancheId, toBytes16(fromHex("010000000000000003")));
assertEq(decodedUser, 0x225ef95fa90f4F7938A5b34234d14768cB4263dd);
assertEq(decodedValidUntil, uint(1657870537));
(uint64 decodedPoolId, bytes16 decodedTrancheId, address decodedUser, uint64 decodedValidUntil) = ConnectorMessages.parseUpdateMember(fromHex("040000000000000002811acd5b3f17c06841c7e41e9e04cb1b12312312312312312312312312312312312312312312312312312312312312310000000065B376AA").ref(0));
assertEq(uint(decodedPoolId), uint(2));
assertEq(decodedTrancheId, hex"811acd5b3f17c06841c7e41e9e04cb1b");
assertEq(decodedUser, 0x1231231231231231231231231231231231231231);
assertEq(decodedValidUntil, uint(1706260138));
}

function testUpdateMemberEquivalence(
uint64 poolId,
bytes16 trancheId,
address user,
uint256 amount
uint64 validUntil
) public {
bytes memory _message = ConnectorMessages.formatUpdateMember(
poolId,
trancheId,
user,
amount
validUntil
);
(
uint64 decodedPoolId,
bytes16 decodedTrancheId,
address decodedUser,
uint256 decodedAmount
uint64 decodedValidUntil
) = ConnectorMessages.parseUpdateMember(_message.ref(0));
assertEq(uint256(decodedPoolId), uint256(poolId));
assertEq(uint(decodedPoolId), uint(poolId));
assertEq(decodedTrancheId, trancheId);
assertEq(decodedUser, user);
assertEq(decodedAmount, amount);
assertEq(uint(decodedValidUntil), uint(validUntil));
}

function testUpdateTokenPriceEncoding() public {
Expand Down
4 changes: 2 additions & 2 deletions test/mock/MockHomeConnector.sol
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -43,8 +43,8 @@ contract MockHomeConnector is Test {
}


function updateMember(uint64 poolId, bytes16 trancheId, address user, uint256 amount) public {
bytes memory _message = ConnectorMessages.formatUpdateMember(poolId, trancheId, user, amount);
function updateMember(uint64 poolId, bytes16 trancheId, address user, uint64 validUntil) public {
bytes memory _message = ConnectorMessages.formatUpdateMember(poolId, trancheId, user, validUntil);
router.handle(_message);
}

Expand Down