주요 결함들
[M-01] YUL CALL RETURN VALUE NOT CHECKED
function _returnDust() private {
uint256 _remainingETH = remainingETH;
assembly {
if gt(_remainingETH, 0) {
let callStatus := call(
gas(),
caller(),
selfbalance(),
0,
0,
0,
0
)
}
}
}_returnDust()는 남은 이더를 msg.sender에게 보내는 함수다. 여기서 문제가 되는 부분은 중간에 있는 call이다. opcode로 실행되는 call의 결과는 callStatus에 저장된다. 그런데 해당 결과의 성공이나 실패 확인 절차 없이 무조건 실행된다는 점이다. 이더를 보낸다고 하더라도 실제로 보냈는지 알 수 없다. 따라서 다음과 같이 리턴 값을 확인하는 코드가 추가되어야 한다.
+ error ReturnDustFail();
+
function _returnDust() private {
uint256 _remainingETH = remainingETH;
+ bool success;
assembly {
if gt(_remainingETH, 0) {
- let callStatus := call(
+ success := call(
gas(),
caller(),
selfbalance(),
0,
0,
0,
0
)
}
}
+ if (!success) revert ReturnDustFail();
}opcode call의 파라미터에 대해 조금 더 알아보자.
gas: The amount of gas to use for the call.
destination: The address of the contract to call.
value: The amount of Ether to send with the call.
in_offset: The offset of the input data in memory.
in_size: The size of the input data in memory.
out_offset: The offset of the output data in memory.
out_size: The size of the output data in memory.
위 설명을 토대로 함수에 적용해보면 다음과 같이 설명할 수 있다.
gas(): gas() 함수를 이용해 현재 사용 가능한 가스의 양을 넣어주고
caller(): 함수를 호출한 주소로 이더를 보낼거니까 caller()를 이용해 주소를 가져온다.
selfbalance(): 현재 컨트랙트의 있는 이더를 모두 전송할 예정이므로 selfbalance()로 컨트랙트의 자금을 value로 넘긴다.
0,0,0,0: input data와 output data가 없으므로 나머지는 모두 0
[M-02] HACKED OWNER OR MALICIOUS OWNER CAN IMMEDIATELY STEAL ALL ASSETS ON THE PLATFORM
contract Exchange is IExchange, ReentrancyGuarded, EIP712, OwnableUpgradeable, UUPSUpgradeable {
//..
}Exchange.sol은 UUPS UPGRADABLE로 즉각적인 컨트랙트 업그레이드가 가능하다. 만약 owner의 권한이 해킹당하거나 malicious owner라면 단 번에 자금 탈취 우려가 있다. UUPSUpgradeable를 사용하는 모든 컨트랙트가 취약하다는 것이 아니다. 현재 Exchange.sol은 유저가 ERC20 / ERC721 / ERC1155 token을 approve 하고 자산을 이동하는 핵심 컨트랙트다. 핵심 로직에 너무 많은 권한이 집중되어 있는 것은 문제가 되며, 권한이 넘어가지 않도록 각별한 주의가 필요하다.
따라서 즉각적인 업그레이드가 불가능하게 타임락을 걸거나 컨트랙트에 너무 많은 권한이 몰리지 않도록 권력이 분산되는 설계가 필요하다. UUPS 방식은 로직 컨트랙트에서 업그레이드 관련 코드가 있는 방식이다. 배포비용이 저렴하지만 유지관리가 비교적 어렵다는 특징이 있다.
[M-03] ALL ORDERS WHICH USE EXPIRATIONTIME == 0 TO SUPPORT ORACLE CANCELLATION ARE NOT EXECUTABLE
function _validateOrderParameters(Order calldata order, bytes32 orderHash)
internal
view
returns (bool)
{
return (
/* Order must have a trader. */
(order.trader != address(0)) &&
/* Order must not be cancelled or filled. */
(!cancelledOrFilled[orderHash]) &&
/* Order must be settleable. */
(order.listingTime < block.timestamp) &&
(block.timestamp < order.expirationTime)
);
}프로젝트 독스를 보면 다음과 같이 나와있다.
Off-chain methods
“Oracle cancellations - if the order is signed with an expirationTime of 0, a user can request an oracle to stop producing authorization signatures; without a recent signature, the order will not be able to be matched”
하지만 order가 유효한지 확인하는 _validateOrderParameters를 보면
(block.timestamp < order.expirationTime)expirationTime이 0인 order는 항상 false를 리턴하면서 유효하지 않은 주문으로 처리될 것이다. 따라서 해당 케이스를 고려해서 새롭게 로직을 짜야한다.
[M-04] POOL DESIGNED TO BE UPGRADEABLE BUT DOES NOT SET OWNER, MAKING IT UN-UPGRADEABLE
contract Pool is IPool, OwnableUpgradeable, UUPSUpgradeable {
// required by the OZ UUPS module
function _authorizeUpgrade(address) internal override onlyOwner {}
//..
}pool.sol도 프록시 패턴을 사용하고 있다. 프록시 패턴을 사용하는 컨트랙트는 initialize 함수를 통해 owner를 설정해주고 initializer modifier를 사용해서 한 번만 호출되도록 해줘야 한다. 같은 프록시 패턴을 사용하는 Exchange.sol 컨트랙트는 아래와 같이 제대로 설정되어 있지만 현재 pool.sol 컨트랙트에는 없다.
constructor() {
_disableInitializers();
}
/* Constructor (for ERC1967) */
function initialize(
IExecutionDelegate _executionDelegate,
IPolicyManager _policyManager,
address _oracle,
uint _blockRange
) external initializer {
__Ownable_init();
//..
}만약 설정을 하지 않았을 경우, upgrade가 제대로 진행되지 않을 것이며, owner 또한 제대로 설정되지 않았기 때문에 _authorizeUpgrade를 호출하지 못 할 것이다.
피드백
call의 리턴값을 제대로 체크하고 있는지 확인하자.- 핵심 로직 및 컨트랙트에 너무 많은 권한이 집중되어 있는지 확인하자.
- 프록시 컨트랙트에 initialize 또는 constructor가 제대로 설정되어 있는지 확인.
