Sandclock contest - cccz's results

Handle

cccz

Vulnerability details

Impact

There is no slippage control on _swapUstToUnderlying of NonUSTStrategy.sol, which expose strategy to sandwich attack.

And since finishRedeemStable lacks access control, anyone can do a sandwich attack by calling the _swapUstToUnderlying function.

    function finishRedeemStable(uint256 idx) public override(BaseStrategy) {
        super.finishRedeemStable(idx);
        _swapUstToUnderlying();
    }
    ...
    function _swapUstToUnderlying() internal {
        uint256 ustBalance = _getUstBalance();
        if (ustBalance > 0) {
            // slither-disable-next-line unused-return
            curvePool.exchange_underlying(ustI, underlyingI, ustBalance, 0);
        }
    }

Proof of Concept

https://github.com/code-423n4/2022-01-sandclock/blob/main/sandclock/contracts/strategy/NonUSTStrategy.sol#L107-L110

https://github.com/code-423n4/2022-01-sandclock/blob/main/sandclock/contracts/strategy/NonUSTStrategy.sol#L90-L96

Tools Used

Manual analysis

Recommended Mitigation Steps

Replace 0 in the parameter of exchange_underlying with minout

Handle

cccz

Vulnerability details

Impact

The mint function of the Depositors contract uses the _safeMint function of the ERC721 contract. And there is a reentrancy vulnerability in the _safeMint function

   function _safeMint(
       address to,
       uint256 tokenId,
       bytes memory _data
   ) internal virtual {
       _mint(to, tokenId);
       require(
           _checkOnERC721Received(address(0), to, tokenId, _data),
           "ERC721: transfer to non ERC721Receiver implementer"
       );
   }
   ...
   function _checkOnERC721Received(
       address from,
       address to,
       uint256 tokenId,
       bytes memory _data
   ) private returns (bool) {
       if (to.isContract()) {
           try IERC721Receiver(to).onERC721Received(_msgSender(), from, tokenId, _data) returns (bytes4 retval) {
               return retval == IERC721Receiver.onERC721Received.selector;

The deposit function of the Vault contract will call the _createDeposit and _createClaim functions, and the mint function of the Depositors contract will be called in the _createClaim function. Because the _transferAndCheckUnderlying function will be called after the mint function to increase the underlying tokens in the contract. The user can reenter the deposit function in the mint function. In the _createDeposit function, since the underlying tokens are less than normal, the localTotalUnderlying variable will be smaller. Later, when the mint function of the Claimers contract is called in the _createClaim function, the user will get more share tokens.

    function deposit(DepositParams calldata _params) external {
        _createDeposit(_params.amount, _params.lockedUntil, _params.claims);
        _transferAndCheckUnderlying(_msgSender(), _params.amount);
    }
    ...
    function _createDeposit(
        uint256 _amount,
        uint256 _lockedUntil,
        ClaimParams[] calldata claims
    ) internal {
        if (_lockedUntil == 0) _lockedUntil = block.timestamp + minLockPeriod;
        else
            require(
                _lockedUntil >= block.timestamp + minLockPeriod,
                "Vault: lock time is too small"
            );

        uint256 localTotalShares = totalShares();
        uint256 localTotalUnderlying = totalUnderlyingMinusSponsored();
        uint256 pct = 0;

        for (uint256 i = 0; i < claims.length; ++i) {
            ClaimParams memory data = claims[i];
            _createClaim(
                _depositGroupIds.current(),
                _amount,
                _lockedUntil,
                data,
                localTotalShares,
                localTotalUnderlying
            );
            pct += data.pct;
        }
    ...
    function _createClaim(
        uint256 _depositGroupId,
        uint256 _amount,
        uint256 _lockedUntil,
        ClaimParams memory _claim,
        uint256 _localTotalShares,
        uint256 _localTotalPrincipal
    ) internal {
        uint256 amount = _amount.percOf(_claim.pct);

        uint256 newShares = _computeShares(
            amount,
            _localTotalShares,
            _localTotalPrincipal
        );

        uint256 claimerId = claimers.mint(
            _claim.beneficiary,
            amount,
            newShares
        );

        uint256 tokenId = depositors.mint(

Proof of Concept

https://github.com/code-423n4/2022-01-sandclock/blob/main/sandclock/contracts/vault/Depositors.sol#L53 https://github.com/code-423n4/2022-01-sandclock/blob/main/sandclock/contracts/Vault.sol#L160-L163 https://github.com/code-423n4/2022-01-sandclock/blob/main/sandclock/contracts/Vault.sol#L438 https://github.com/code-423n4/2022-01-sandclock/blob/main/sandclock/contracts/Vault.sol#L476

Tools Used

Manual analysis

Recommended Mitigation Steps

Use the _mint function of the ERC721 contract in the mint function of the Depositors contract.