code4rena-2023-11-party-dao-h01
[H-01] The 51% majority can hijack the party’s precious tokens through an arbitrary call proposal if the AddPartyCardsAuthority contract is added as an authority in the party
Summary
본래 만장일치여야 precious NFT를 이동할 수 있다. 하지만 NFT(투표권)를 새로 발행하는 함수 호출을 막지 않았기에 51%의 동의만으로 많은 투표권을 찍어내고, 만장일치인 상황을 만들어내어 NFT를 탈취할 수 있다.
Keyword
51% attack, dao, theft
Vulnerability
- proposals/ProposalExecutionEngine.sol#L260-L264
- proposals/ArbitraryCallsProposal.sol
- authorities/AddPartyCardsAuthority.sol#L29
취약점에 대해 알아보기 전 먼저 숙지할 것이 있다.
- AddPartyCardsAuthority 컨트랙트: 이 컨트랙트는 Party DAO에 Authority로 추가되는 컨트랙트이다(기능을 사용할 경우 추가해야 함). 이 컨트랙트는 새로운 Party NFT를 발행하는 기능을 제공한다. 현재 메인넷의 0xC534bb3640A66fAF5EAE8699FeCE511e1c331cAD 에 배포되어 있다.
- ArbitraryCallsProposal 컨트랙트: ProposalExecutionEngine 컨트랙트에서 상속하는 컨트랙트로, 임의의 컨트랙트콜을 실행하는 기능을 제공한다. PartyDAO는 ArbitraryCallsProposal를 이용하는 제안에는 많은 제한을 두어 51%(과반수)가 동의하더라도 precious NFT를 훔칠 수 없도록 안전장치를 마련했다. precious NFT를 이동하기 위해서는 DAO 멤버들이 만장일치로 투표해야 한다.
ArbitraryCallsProposal 컨트랙트는 ProposalExecutionEngine 컨트랙트에서 상속하는 컨트랙트로, 임의의 컨트랙트콜을 실행하는 기능을 제공한다. ProposalType.ArbitraryCalls 제안이 투표를 통해 통과하면 임의의 콜을 할 수 있다.
function _execute(
ProposalType pt,
ExecuteProposalParams memory params
) internal virtual returns (bytes memory nextProgressData) {
if (pt == ProposalType.ListOnOpensea) {
...
@> } else if (pt == ProposalType.ArbitraryCalls) {
@> nextProgressData = _executeArbitraryCalls(
params,
_getSharedProposalStorage().opts.allowArbCallsToSpendPartyEth
);
}
...
}_executeArbitraryCalls 에서 임의의 콜을 실행할 때 투표 결과가 만장일치라면 precious 토큰을 이동할 수 있고, 만장일치가 아니라면 이동할 수 없다. 또한 approve 등의 함수를 막는 등 위험한 작업을 막고 있다.
function _executeArbitraryCalls(
IProposalExecutionEngine.ExecuteProposalParams memory params,
bool allowArbCallsToSpendPartyEth
) internal returns (bytes memory nextProgressData) {
// Get the calls to execute.
ArbitraryCall[] memory calls = abi.decode(params.proposalData, (ArbitraryCall[]));
// Check whether the proposal was unanimously passed.
bool isUnanimous = params.flags & LibProposal.PROPOSAL_FLAG_UNANIMOUS ==
LibProposal.PROPOSAL_FLAG_UNANIMOUS;
// If not unanimous, keep track of which preciouses we had before the calls
// so we can check that we still have them later.
bool[] memory hadPreciouses = new bool[](params.preciousTokenIds.length);
@> if (!isUnanimous) {
for (uint256 i; i < hadPreciouses.length; ++i) {
hadPreciouses[i] = _getHasPrecious(
params.preciousTokens[i],
params.preciousTokenIds[i]
);
}
}
// If we're not allowing arbitrary calls to spend the Party's ETH, only
// allow forwarded ETH attached to the call to be spent.
uint256 ethAvailable = allowArbCallsToSpendPartyEth ? address(this).balance : msg.value;
for (uint256 i; i < calls.length; ++i) {
// Execute an arbitrary call.
@> _executeSingleArbitraryCall(
i,
calls,
params.preciousTokens,
params.preciousTokenIds,
isUnanimous,
ethAvailable
);
// Update the amount of ETH available for the subsequent calls.
ethAvailable = allowArbCallsToSpendPartyEth
? address(this).balance
: ethAvailable - calls[i].value;
emit ArbitraryCallExecuted(params.proposalId, i, calls.length);
}
// If not a unanimous vote and we had a precious beforehand,
// ensure that we still have it now.
@> if (!isUnanimous) {
for (uint256 i; i < hadPreciouses.length; ++i) {
if (hadPreciouses[i]) {
if (!_getHasPrecious(params.preciousTokens[i], params.preciousTokenIds[i])) {
revert PreciousLostError(
params.preciousTokens[i],
params.preciousTokenIds[i]
);
}
}
}
}
// Refund leftover ETH attached to the call if none was spent from the
// Party's balance.
if (!allowArbCallsToSpendPartyEth && ethAvailable > 0) {
payable(msg.sender).transferEth(ethAvailable);
}
// No next step, so no progressData.
return "";
}
function _executeSingleArbitraryCall(
uint256 idx,
ArbitraryCall[] memory calls,
IERC721[] memory preciousTokens,
uint256[] memory preciousTokenIds,
bool isUnanimous,
uint256 ethAvailable
) private {
ArbitraryCall memory call = calls[idx];
// Check that the call is not prohibited.
if (
@> !_isCallAllowed(call, isUnanimous, idx, calls.length, preciousTokens, preciousTokenIds)
) {
revert CallProhibitedError(call.target, call.data);
}
// Check that we have enough ETH to execute the call.
if (ethAvailable < call.value) {
revert NotEnoughEthAttachedError(call.value, ethAvailable);
}
// Execute the call.
@> (bool s, bytes memory r) = call.target.call{ value: call.value }(call.data);
if (!s) {
// Call failed. If not optional, revert.
revert ArbitraryCallFailedError(r);
} else {
// Call succeeded.
// If we have a nonzero expectedResultHash, check that the result data
// from the call has a matching hash.
if (call.expectedResultHash != bytes32(0)) {
bytes32 resultHash = keccak256(r);
if (resultHash != call.expectedResultHash) {
revert UnexpectedCallResultHashError(idx, resultHash, call.expectedResultHash);
}
}
}
}
function _isCallAllowed(
ArbitraryCall memory call,
bool isUnanimous,
uint256 callIndex,
uint256 callsCount,
IERC721[] memory preciousTokens,
uint256[] memory preciousTokenIds
) private view returns (bool isAllowed) {
// Cannot call ourselves.
if (call.target == address(this)) {
return false;
}
if (call.data.length >= 4) {
// Get the function selector of the call (first 4 bytes of calldata).
bytes4 selector;
{
bytes memory callData = call.data;
assembly {
selector := and(
mload(add(callData, 32)),
0xffffffff00000000000000000000000000000000000000000000000000000000
)
}
}
// Non-unanimous proposals restrict what ways some functions can be
// called on a precious token.
@> if (!isUnanimous) {
// Cannot call `approve()` or `setApprovalForAll()` on the precious
// unless it's to revoke approvals.
@> if (selector == IERC721.approve.selector) {
// Can only call `approve()` on the precious if the operator is null.
(address op, uint256 tokenId) = _decodeApproveCallDataArgs(call.data);
if (op != address(0)) {
return
!LibProposal.isTokenIdPrecious(
IERC721(call.target),
tokenId,
preciousTokens,
preciousTokenIds
);
}
// Can only call `setApprovalForAll()` on the precious if
// toggling off.
@> } else if (selector == IERC721.setApprovalForAll.selector) {
(, bool isApproved) = _decodeSetApprovalForAllCallDataArgs(call.data);
if (isApproved) {
return !LibProposal.isTokenPrecious(IERC721(call.target), preciousTokens);
}
// Can only call cancelAuction on the zora AH if it's the last call
// in the sequence.
@> } else if (selector == IReserveAuctionCoreEth.cancelAuction.selector) {
if (call.target == address(_ZORA)) {
return callIndex + 1 == callsCount;
}
}
}
// Can never call receive hooks on any target.
if (
@> selector == IERC721Receiver.onERC721Received.selector ||
@> selector == ERC1155TokenReceiverBase.onERC1155Received.selector ||
@> selector == ERC1155TokenReceiverBase.onERC1155BatchReceived.selector
) {
return false;
}
// Disallow calling `validate()` on Seaport if there are preciouses.
if (selector == IOpenseaExchange.validate.selector && preciousTokens.length != 0) {
return false;
}
}
// All other calls are allowed.
return true;
}하지만 이 함수에서는 AddPartyCardsAuthority.addPartyCards 함수의 호출은 막지 않는다. 따라서 51%가 동의하여 NFT를(표를) 찍어낼 수 있다.
function addPartyCards(
address[] calldata newPartyMembers,
@> uint96[] calldata newPartyMemberVotingPowers,
address[] calldata initialDelegates
) external {
uint256 newPartyMembersLength = newPartyMembers.length;
if (newPartyMembersLength == 0) {
revert NoPartyMembers();
}
if (
newPartyMembersLength != newPartyMemberVotingPowers.length ||
newPartyMembersLength != initialDelegates.length
) {
revert ArityMismatch();
}
uint96 addedVotingPower;
for (uint256 i; i < newPartyMembersLength; ++i) {
if (newPartyMemberVotingPowers[i] == 0) {
revert InvalidPartyMemberVotingPower();
}
if (newPartyMembers[i] == address(0)) {
revert InvalidPartyMember();
}
addedVotingPower += newPartyMemberVotingPowers[i];
}
Party(payable(msg.sender)).increaseTotalVotingPower(addedVotingPower);
for (uint256 i; i < newPartyMembersLength; ++i) {
address newPartyMember = newPartyMembers[i];
uint96 newPartyMemberVotingPower = newPartyMemberVotingPowers[i];
PartyGovernanceNFT(msg.sender).mint(
newPartyMember,
@> newPartyMemberVotingPower,
initialDelegates[i]
);
emit PartyCardAdded(msg.sender, newPartyMember, newPartyMemberVotingPower);
}
}99.99% 이상이면 만장일치로 보기 때문에, 굉장히 많은 표를 찍어내면 다음 투표에서는 찍어낸 표를 이용하여 만장일치를 만들 수 있다. 이를 통해 51%가 NFT를 탈취하는 시나리오가 가능하다.
function _isUnanimousVotes(
uint96 totalVotes,
uint96 totalVotingPower
) private pure returns (bool) {
uint256 acceptanceRatio = (totalVotes * 1e4) / totalVotingPower;
// If >= 99.99% acceptance, consider it unanimous.
// The minting formula for voting power is a bit lossy, so we check
// for slightly less than 100%.
@> return acceptanceRatio >= 0.9999e4;
}Impact
만장일치가 아닌 51%만으로 precious NFT를 꺼내갈 수 있다. NFT를 탈취한다.
Mitigation
ArbitraryCallsProposal._isCallAllowed 에서 AddPartyCardsAuthority.addPartyCards 호출을 막는다.
Memo
위에서 언급되는 컨트랙트들은 스코프 표에 명확히 포함되지는 않았다. 하지만 스코프에 포함되는 ProposalExecutionEngine 컨트랙트에서 ArbitraryCallsProposal를 상속하므로 포함된다고 볼 수 있다. 표에 없는 컨트랙트라 지나쳤는데, 상속된 컨트랙트도 잘 살펴볼 필요가 있겠다.. 또한 여유가 있다면 스코프 이상을 파악해보자.
‘51%가 동의하더라도 불가한 일을 51%가 성공해낸다’ 라는 발상에 초점을 맞추고 떠올린 것 같다.
tags: bughunting, party dao, smart contract, solidity, dao, 51% attack, crypto theft, nft, severity high