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.

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

Remediate malformed return during the twin transfer #771

Merged
merged 4 commits into from
Aug 28, 2023
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
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
35 changes: 35 additions & 0 deletions contracts/mock/Foreign1155.sol
Original file line number Diff line number Diff line change
Expand Up @@ -38,3 +38,38 @@ contract Foreign1155GasTheft is Foreign1155 {
}
}
}

/*
* @title Foreign1155 that succeeds, but the data cannot be decoded into a boolean
*
* @notice Mock ERC-(1155) for Unit Testing
*/
contract Foreign1155MalformedReturn is Foreign1155 {
enum AttackType {
ReturnTooShort,
ReturnTooLong,
ReturnInvalid
}

AttackType public attackType;

function setAttackType(AttackType _attackType) external {
attackType = _attackType;
}

function safeTransferFrom(address, address, uint256, uint256, bytes memory) public virtual override {
if (attackType == AttackType.ReturnTooShort) {
assembly {
return(0, 31) // return too short data
}
} else if (attackType == AttackType.ReturnTooLong) {
assembly {
return(0, 33) // return too long data
}
} else if (attackType == AttackType.ReturnInvalid) {
assembly {
return(0x40, 32) // return a value that is not 0 or 1
}
}
}
}
50 changes: 50 additions & 0 deletions contracts/mock/Foreign20.sol
Original file line number Diff line number Diff line change
Expand Up @@ -246,3 +246,53 @@ contract Foreign20GasTheft is Foreign20 {
return false;
}
}

/*
* @title Foreign20 that returns an absurdly long return data
*
* @notice Mock ERC-(20) for Unit Testing
*/
contract Foreign20ReturnBomb is Foreign20 {
function transferFrom(address, address, uint256) public virtual override returns (bool) {
assembly {
revert(0, 3000000)
// This is carefully chosen. If it's too low, not enough gas is consumed and contract that call it does not run out of gas.
// If it's too high, then this contract runs out of gas before the return data is returned.
}
}
}

/*
* @title Foreign20 that succeeds, but the data cannot be decoded into a boolean
*
* @notice Mock ERC-(20) for Unit Testing
*/
contract Foreign20MalformedReturn is Foreign20 {
enum AttackType {
ReturnTooShort,
ReturnTooLong,
ReturnInvalid
}

AttackType public attackType;

function setAttackType(AttackType _attackType) external {
attackType = _attackType;
}

function transferFrom(address, address, uint256) public virtual override returns (bool) {
if (attackType == AttackType.ReturnTooShort) {
assembly {
return(0, 31) // return too short data
}
} else if (attackType == AttackType.ReturnTooLong) {
assembly {
return(0, 33) // return too long data
}
} else if (attackType == AttackType.ReturnInvalid) {
assembly {
return(0x40, 32) // return a value that is not 0 or 1
}
}
}
}
35 changes: 35 additions & 0 deletions contracts/mock/Foreign721.sol
Original file line number Diff line number Diff line change
Expand Up @@ -44,3 +44,38 @@ contract Foreign721GasTheft is Foreign721 {
}
}
}

/*
* @title Foreign721 that succeeds, but the data cannot be decoded into a boolean
*
* @notice Mock ERC-(721) for Unit Testing
*/
contract Foreign721MalformedReturn is Foreign721 {
enum AttackType {
ReturnTooShort,
ReturnTooLong,
ReturnInvalid
}

AttackType public attackType;

function setAttackType(AttackType _attackType) external {
attackType = _attackType;
}

function safeTransferFrom(address, address, uint256, bytes memory) public virtual override {
if (attackType == AttackType.ReturnTooShort) {
assembly {
return(0, 31) // return too short data
}
} else if (attackType == AttackType.ReturnTooLong) {
assembly {
return(0, 33) // return too long data
}
} else if (attackType == AttackType.ReturnInvalid) {
assembly {
return(0x40, 32) // return a value that is not 0 or 1
}
}
}
}
5 changes: 4 additions & 1 deletion contracts/protocol/facets/ExchangeHandlerFacet.sol
Original file line number Diff line number Diff line change
Expand Up @@ -898,7 +898,10 @@ contract ExchangeHandlerFacet is IBosonExchangeHandler, BuyerBase, DisputeBase {
bytes memory result;
(success, result) = twinM.tokenAddress.call{ gas: gasleft() - reservedGas }(data);

success = success && (result.length == 0 || abi.decode(result, (bool)));
// Check if result is empty or if result is a boolean and is true
success =
success &&
(result.length == 0 || (result.length == 32 && abi.decode(result, (uint256)) == 1));
}
}

Expand Down
Loading